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

[Feature]: Compatibility for bigger Shulkers #33

Closed
sternschnaube opened this issue May 21, 2024 · 14 comments
Closed

[Feature]: Compatibility for bigger Shulkers #33

sternschnaube opened this issue May 21, 2024 · 14 comments
Labels
bug Something isn't working

Comments

@sternschnaube
Copy link

Request Description

Carpet AMS Addition has a feature to double the Shulker size.
But the overlay isn't compatible with it.

https://github.com/Minecraft-AMS/Carpet-AMS-Addition

Images

image

The progess bar doesn't scale with the bigger size.
image

Fabric Version

0.15.11

Additional Context

No response

@BVengo
Copy link
Owner

BVengo commented Jun 14, 2024

Thanks for this. I'll have to look into it and see how they modify the stack size - hopefully it's in an easily compatible way!

@BVengo BVengo added the bug Something isn't working label Jun 14, 2024
@sternschnaube
Copy link
Author

Thank you 😊

@BVengo
Copy link
Owner

BVengo commented Jul 11, 2024

I was looking into this for 1.21, but unfortunately I would need to implement a hard an inefficient work around for the mod you mentioned. I've opened an issue in their repo which allows for an easy compatibility fix though, so we'll see how it goes.

I'll leave this issue open for now - hopefully we can get this working! 1.21 will be released without it though, since my release is already quite late.

@wendavid552
Copy link

I was looking into this for 1.21, but unfortunately I would need to implement a hard an inefficient work around for the mod you mentioned. I've opened an issue in their repo which allows for an easy compatibility fix though, so we'll see how it goes.

I'll leave this issue open for now - hopefully we can get this working! 1.21 will be released without it though, since my release is already quite late.

One of a possible but inefficient method could be calculate the capacity through .stream() instead of .iterateNonEmpty() as this would go through all the slots, including the empty ones here (btw I haven't tested it).

@BVengo
Copy link
Owner

BVengo commented Jul 11, 2024

Yep, I tested that but, strangely, it still only returned non-empty. I suspect there are two different shulker inventories - one for the held item ContainerComponent which only knows the exact contents (what we're handling), and one for the block itself which has the maximum count (which their mod alters).

Their solution involved changing the size() function, but that's per-instance rather than for all shulkers. My hacky solution would be to create a temporary instance and call size(), but this would be super inefficient and have to be done every single tick in case the setting is changed between large and small at some stage.

Ideally, they just update the public static INVENTORY_SIZE variable (or whatever it was called) alongside their other changes for compatibility with other mods.

@sternschnaube
Copy link
Author

Thank you both for your efforts 😊

@wendavid552
Copy link

Yep, I tested that but, strangely, it still only returned non-empty. I suspect there are two different shulker inventories - one for the held item ContainerComponent which only knows the exact contents (what we're handling), and one for the block itself which has the maximum count (which their mod alters).

Their solution involved changing the size() function, but that's per-instance rather than for all shulkers. My hacky solution would be to create a temporary instance and call size(), but this would be super inefficient and have to be done every single tick in case the setting is changed between large and small at some stage.

Ideally, they just update the public static INVENTORY_SIZE variable (or whatever it was called) alongside their other changes for compatibility with other mods.

XD I am one of the contributor from Carpet-AMS-Addition😊.

The problem is, our mod is a serverside mod, while simple-shulker-preview is a clientside mod, modifying the constant serverside won't help.

@BVengo
Copy link
Owner

BVengo commented Jul 11, 2024

XD I am one of the contributor from Carpet-AMS-Addition😊.

The problem is, our mod is a serverside mod, while simple-shulker-preview is a clientside mod, modifying the constant serverside won't help.

Oh dang, I never considered that. That puts a spanner in the works!

Will have to see if I can somehow get to the size() function from the objects I have available, because I'd really like to avoid my hacky solution.

Alternatively, I could add a custom config for shulker size modification. Not difficult to do, but I'd like to keep it as generic as possible so I don't like that solution either.

@wendavid552
Copy link

wendavid552 commented Jul 11, 2024

XD I am one of the contributor from Carpet-AMS-Addition😊.
The problem is, our mod is a serverside mod, while simple-shulker-preview is a clientside mod, modifying the constant serverside won't help.

Oh dang, I never considered that. That puts a spanner in the works!

Will have to see if I can somehow get to the size() function from the objects I have available, because I'd really like to avoid my hacky solution.

Alternatively, I could add a custom config for shulker size modification. Not difficult to do, but I'd like to keep it as generic as possible so I don't like that solution either.

Hi I wrote a quick fix in #35 by .stream(). It works actually.

If you want to test it with Carpet-AMS-Addition, I have sent a test server to you in Discord.

@BVengo
Copy link
Owner

BVengo commented Jul 13, 2024

Thanks for that. Did some more testing and added discussion to the PR. Unfortunately I'm still not seeing .stream() work!

@sternschnaube
Copy link
Author

Any update on this @BVengo? 😊

@BVengo
Copy link
Owner

BVengo commented Oct 8, 2024

Unfortunately not, sorry about that. Life has kept me too busy for non-update development recently.

It's top on my list for bug fixes though, when I'm available again!

@sternschnaube
Copy link
Author

Thanks for letting me know 😄

@BVengo BVengo closed this as completed in 21e4bd9 Nov 12, 2024
@BVengo
Copy link
Owner

BVengo commented Nov 12, 2024

I've put a lot of time into investigating this and I'm afraid there's really no way to dynamically handle this until Carpet-AMS-Addition adds a method of communicating with client-side mods. See Minecraft-AMS/Carpet-AMS-Addition#130 for their progress on this.

From a client-side perspective, the inventory simply stores a list of items on the player but does not save the size of the container. The size is handled on the server-side, so client-side mods are stuck assuming that it's 27 stacks as per the vanilla size.

As a workaround, I've now implemented two new options under the Compatibility section allowing you to tell the mod what the size of your shulker boxes really are. There are two downsides to this though.

  1. It effects all shulker boxes - because of the server/client-side communications listed above, there is no way to dynamically adapt for shulker boxes of varying sizes
  2. This mod is designed to be a QoL feature that you set and forget, so configs are per-instance rather than per-server. If you swap between servers with and without the larger shulkers, you'll find yourself needing to change these settings regularly.

This is being implemented from 1.21.3 onwards. Unfortunately I'm rather pressed for time recently, so I may not get a chance to backdate these features. Thank you for your patience on this issue though 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants