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

Use immutable collections for settings #3539

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ZacSharp
Copy link
Collaborator

@ZacSharp ZacSharp commented Jul 3, 2022

Using an ImmutableSet seems to be faster according to this benchmark, but that would either require a breaking API change or improperly making the settings ImmutableSets while exposing only their list views (i.e. lists that behave as ordered sets, which currently works because we only use them as ordered sets).

@leijurv
Copy link
Member

leijurv commented Jul 5, 2022

Seems like this would really help with #3479. Are there any downsides?

Also I don't care about breaking API changes as long as impact client isn't affected 😈 (joke) (but only sorta)

@ZacSharp
Copy link
Collaborator Author

Others can still put in their own implementations like

BaritoneAPI.getSettings().acceptableThrowawayItems.value = new ArrayList<>();
// some time later
BaritoneAPI.getSettings().acceptableThrowawayItems.value.add(net.minecraft.init.Blocks.DIAMOND_BLOCK);

or even the unrealistic

BaritoneAPI.getSettings().acceptableThrowawayItems.value = new MyCursedListThatRandomlyRepopulatesOnEveryAccess<>();

which both screw up the caching and in the latter case even the current implementation.
Apart from that there's nothing coming to my mind.

@ZacSharp
Copy link
Collaborator Author

ZacSharp commented Oct 4, 2022

Is any of those scenarios or something else holding this back?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants