Skip to content
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.

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

Transfer API: Add slotted storage and non-empty iterator #2908

Merged
merged 10 commits into from
Apr 11, 2023

Conversation

Technici4n
Copy link
Member

@Technici4n Technici4n commented Feb 14, 2023

  • Add non-empty iterator system. This allows many slots to be skipped in large inventories provided a suitable tracking structure. Adds StorageUtil.extractAny.
  • Add SlottedStorage that allows storages to optionally expose a slot list. This is optional, and is meant for usage by chest-like inventories.
  • Clarify edge case in SingleVariantStorage#getBlankVariant.
  • Remove unused field from TransferApiImpl.

None of these should be breaking changes.

@Technici4n Technici4n changed the base branch from 1.19.3 to 1.19.4 March 31, 2023 17:32
@Technici4n Technici4n requested review from sfPlayer1, modmuss50 and a team March 31, 2023 17:40
Copy link
Contributor

@SquidDev SquidDev left a comment

Choose a reason for hiding this comment

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

Thanks!

* for example {@link StorageUtil#insertStacking} or {@link ContainerItemContext#getAdditionalSlots()}.
*
* <p>It is guaranteed that calling this function is fast.
* The default implementation returns a view over the storage that delegates to {@link #getSlotCount} and {@link #getSlot}.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have @implSpec here for specifying the default impl? (Yarn adds it to its list of javadoc tags at least)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly we don't.

Copy link
Member Author

Choose a reason for hiding this comment

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

(They just vanish if you run gradlew javadoc...)

Copy link
Member

Choose a reason for hiding this comment

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

Probably a good improvement for a future PR, they're really easy to add:
https://github.com/FabricMC/yarn/blob/4eed62e0827b13c25af0a5b1262fb204c9ddbf52/build.gradle#L554-L558

@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Apr 2, 2023
@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Apr 10, 2023
@modmuss50 modmuss50 merged commit d51205d into FabricMC:1.19.4 Apr 11, 2023
modmuss50 pushed a commit that referenced this pull request Apr 11, 2023
* Transfer API: Add non-empty iterator

* Add SlottedStorage

* Add StorageUtil.extractAny

* Undeprecate ContainerItemContext.withInitial

* Add licenses

* Revert "Undeprecate ContainerItemContext.withInitial"

This reverts commit dcf123e.

* Tweaks

* Make SlottedStorage#getSlots return a view, remove useless field, add UnmodifiableView annotations

* Remove useless @inheritdoc

* Fix infinite loop in the tests
qouteall pushed a commit to qouteall/fabric that referenced this pull request May 3, 2023
* Transfer API: Add non-empty iterator

* Add SlottedStorage

* Add StorageUtil.extractAny

* Undeprecate ContainerItemContext.withInitial

* Add licenses

* Revert "Undeprecate ContainerItemContext.withInitial"

This reverts commit dcf123e.

* Tweaks

* Make SlottedStorage#getSlots return a view, remove useless field, add UnmodifiableView annotations

* Remove useless @inheritdoc

* Fix infinite loop in the tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants