Skip to content

Adjust tank indexing in the Transposer's transferFluid method #3659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master-MC1.12
Choose a base branch
from
Open

Adjust tank indexing in the Transposer's transferFluid method #3659

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/scala/li/cil/oc/server/component/traits/InventoryTransfer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ trait InventoryTransfer extends traits.WorldAware with traits.SideRestricted {
val sinkSide = checkSideForAction(args, 1)
val sinkPos = position.offset(sinkSide)
val count = args.optFluidCount(2)
val sourceTank = args.optInteger(3, -1)
val sourceTank = args.optInteger(3, -1) - 1
Copy link
Contributor

@asiekierka asiekierka Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@repo-alt What do you think? This is a change we pulled from GT:NH in 2022, and using 1-based indexes does make sense, but how much production code would breaking this break?

Copy link
Contributor

@repo-alt repo-alt Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will break some code of course, and there is no easy way to notify the user about that change, but the change makes sense indeed, so I don't think you should look back at us.

Copy link
Contributor

@asiekierka asiekierka Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. In this case I'll probably keep it for a larger release, like 1.9.0 (if it ever happens - at this rate, I doubt it!).


onTransferContents() match {
case Some(reason) =>
Expand Down