Team-EnderIO/EnderIO

Machine fluid tank update branch #113

Closed

albinaask wants to merge 0 commits from machine-fluid-tank-update-branch into dev/1.19.x

albinaask commented

2023-03-26 17:59:32 +00:00

(Migrated from github.com)

Description

Fluid tanks now behave more like they behaved in 1.12, They can be clicked on with buckets or other containers to transfer fluid, their item correspondence are now correctly fluid containing items with accessible fluid stacks. The item variant now renders fluid and displays tank content on the extended tooltip which you can access by holding shift.

Fix the break particles of the tanks, I tried but didn't get this to work.
Allow tanks to stack to 64. I worked on this for like 12h, but only went around in circles. It seems to be rather involved. (This I think can be fixed later since it is rather involved)

Checklist:

My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation

Description Fluid tanks now behave more like they behaved in 1.12, They can be clicked on with buckets or other containers to transfer fluid, their item correspondence are now correctly fluid containing items with accessible fluid stacks. The item variant now renders fluid and displays tank content on the extended tooltip which you can access by holding shift. - [x] Fix the break particles of the tanks, I tried but didn't get this to work. - [ ] Allow tanks to stack to 64. I worked on this for like 12h, but only went around in circles. It seems to be rather involved. (This I think can be fixed later since it is rather involved) Checklist: - [x]My code follows the style guidelines of this project (.editorconfig, most IDEs will use this for you) - [x]I have performed a self-review of my own code - [x]I have commented my code, particularly in hard-to-understand areas - [x]I have made corresponding changes to the documentation

Rover656 (Migrated from github.com) requested changes

2023-03-30 01:28:12 +00:00

Rover656 (Migrated from github.com) left a comment

Just a quick scan, I don't have the time for reading over your fluid logic however so I'll have to defer that to another reviewer. Nice work overall, just caught a handful of things that already have implementations that you've sort of partially reimplemented.

Just a quick scan, I don't have the time for reading over your fluid logic however so I'll have to defer that to another reviewer. Nice work overall, just caught a handful of things that already have implementations that you've sort of partially reimplemented.

src/core/java/com/enderio/core/client/item/ItemTooltip.java

Rover656 (Migrated from github.com) commented

2023-03-30 01:20:36 +00:00

We already have a lot of tooltip functionality as a part of the IAdvancedTooltipProvider interface, I'd recommend a little read over how this works, most of the tools use it.

We already have a lot of tooltip functionality as a part of the IAdvancedTooltipProvider interface, I'd recommend a little read over how this works, most of the tools use it.

Rover656 (Migrated from github.com) commented

2023-03-30 01:20:58 +00:00

As a part of my prior comment, this would then be performed inside of the item itself.

As a part of my prior comment, this would then be performed inside of the item itself.

src/machines/java/com/enderio/machines/common/io/fluid/MachineFluidTank.java

Rover656 (Migrated from github.com) commented

2023-03-30 01:24:19 +00:00

What is the rationale behind a default capacity? Surely we can just expect a capacity to be provided always?

What is the rationale behind a default capacity? Surely we can just expect a capacity to be provided always?

Rover656 (Migrated from github.com) commented

2023-03-30 01:24:30 +00:00

Spacing if this constructor is kept :)

Spacing if this constructor is kept :)

Rover656 (Migrated from github.com) commented

2023-03-30 01:25:07 +00:00

When writing JavaDocs, you don't need this hyphen as that'll then be rendered in with the parameter descriptions in an IDE.

When writing JavaDocs, you don't need this hyphen as that'll then be rendered in with the parameter descriptions in an IDE.

Rover656 (Migrated from github.com) commented

2023-03-30 01:25:44 +00:00

Spacing/Newlines on this file are a little wonky, might want to auto-format.

Spacing/Newlines on this file are a little wonky, might want to auto-format.

src/machines/java/com/enderio/machines/common/item/FluidTankItem.java

Rover656 (Migrated from github.com) commented

2023-03-30 01:26:10 +00:00

Why would we allow fluid tank stacking?

Why would we allow fluid tank stacking?

src/machines/java/com/enderio/machines/common/item/FluidTankItem.java

Rover656 (Migrated from github.com) commented

2023-03-30 01:26:36 +00:00

Could you clarify what you mean by this?

Could you clarify what you mean by this?

src/main/java/com/enderio/base/common/blockentity/IWrenchable.java

Rover656 (Migrated from github.com) commented

2023-03-30 01:26:54 +00:00

Spacing on the end there :)

Spacing on the end there :)

src/main/java/com/enderio/base/common/item/tool/YetaWrenchItem.java

					
				@ -34,3 +40,4 @@

				            return InteractionResult.SUCCESS;

				        Level level = pContext.getLevel();

				        BlockPos pos = pContext.getClickedPos();

Rover656 (Migrated from github.com) commented

2023-03-30 01:27:21 +00:00

On a return, we don't generally have to put an else, but its stylistic choice.
Also check your spacing :)

On a return, we don't generally have to put an else, but its stylistic choice. Also check your spacing :)

albinaask commented

2023-04-02 19:43:50 +00:00

(Migrated from github.com)

I don't know what the fuck I'm doing wrong. Event this branch broke down for some inexplicable reason. I'm truly sorry for the pain this causes you

I don't know what the fuck I'm doing wrong. Event this branch broke down for some inexplicable reason. I'm truly sorry for the pain this causes you

albinaask (Migrated from github.com) reviewed

2023-04-02 20:53:44 +00:00

src/machines/java/com/enderio/machines/common/io/fluid/MachineFluidTank.java

albinaask (Migrated from github.com) commented

2023-04-02 20:53:43 +00:00

The rationale is that we probably want a default unified size for all machines, we wouldn't want the vat to have tanks with 16 buckets size, a combustion generator with 5, and other machines with other random numbers added at different points in time. We would like it to be set to one default value and then if there is a specific reason you may change it to change a specific need of a specific machine. Now I removed the field and just inlined it into one of the constructors.

The rationale is that we probably want a default unified size for all machines, we wouldn't want the vat to have tanks with 16 buckets size, a combustion generator with 5, and other machines with other random numbers added at different points in time. We would like it to be set to one default value and then if there is a specific reason you may change it to change a specific need of a specific machine. Now I removed the field and just inlined it into one of the constructors.

albinaask (Migrated from github.com) reviewed

2023-04-02 21:02:51 +00:00

src/machines/java/com/enderio/machines/common/item/FluidTankItem.java

albinaask (Migrated from github.com) commented

2023-04-02 21:02:51 +00:00

It's a feature in 1.12 and is one of my absolute favourite features of EnderIO. It makes the tanks of enderIO the most powerful fluid storing items in modded minecraft in my opinion!

It's a feature in 1.12 and is one of my absolute favourite features of EnderIO. It makes the tanks of enderIO the most powerful fluid storing items in modded minecraft in my opinion!

albinaask (Migrated from github.com) reviewed

2023-04-02 21:12:32 +00:00

src/machines/java/com/enderio/machines/common/item/FluidTankItem.java

albinaask (Migrated from github.com) commented

2023-04-02 21:12:32 +00:00

Sure, since the tag name "fluid" is written to and read from in both the item variant and the block variant, as well as in many other parts of the mod. If you change it in one place, you are going to break the link between the two types. This bug would probably go unnoticed since it would only void fluids under very specific circumstances. So keeping the tag name in one, global place would greatly reduce this risk. It would simply be neater.

Sure, since the tag name "fluid" is written to and read from in both the item variant and the block variant, as well as in many other parts of the mod. If you change it in one place, you are going to break the link between the two types. This bug would probably go unnoticed since it would only void fluids under very specific circumstances. So keeping the tag name in one, global place would greatly reduce this risk. It would simply be neater.

Rover656 commented

2023-04-02 21:13:58 +00:00

(Migrated from github.com)

Oh no; do you have any copies of your work? If not I can try and help you to restore the branch.

Oh no; do you have any copies of your work? If not I can try and help you to restore the branch.

albinaask commented

2023-04-02 21:15:51 +00:00

(Migrated from github.com)

Yes. The work is fine, the branch is dead. I'll make a new pr in a few minutes with fixes to your comments

Yes. The work is fine, the branch is dead. I'll make a new pr in a few minutes with fixes to your comments

Rover656 commented

2023-04-02 21:16:31 +00:00

(Migrated from github.com)

I'm glad to hear you've not lost any of your hard work; no rush with any of this, I just wanted to ask if I could help at all :)

I'm glad to hear you've not lost any of your hard work; no rush with any of this, I just wanted to ask if I could help at all :)

albinaask commented

2023-04-02 21:18:50 +00:00

(Migrated from github.com)

Next time I have to rebase I'll gladly take your help! It seem like it is there I butcher git bigtime...

Next time I have to rebase I'll gladly take your help! It seem like it is there I butcher git bigtime...

albinaask commented

2023-04-02 21:22:56 +00:00

(Migrated from github.com)

Okay. PR is live now, let me know if there are any more changes that is to be made. I really like the feedback since I haven't really coded in Java since like Java 8. And never on this level of quality or in this code base.

Okay. PR is live now, let me know if there are any more changes that is to be made. I really like the feedback since I haven't really coded in Java since like Java 8. And never on this level of quality or in this code base.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No labels

Area-Mod Compat

Good first issue

Status-Awaiting Response

Status-Behind-Flag

Status-Cannot Reproduce

Status-Duplicate

Status-Help Wanted

Status-Incomplete Report

Status-Needs LTS Backport

Status-Needs Updating

Status-To Implement

Status-Wontmerge

Type-Documentation

Type-Enhancement

Type-Suggestion

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Team-EnderIO/EnderIO#113

No description provided.