shedaniel / cloth-config

Client-Sided API for Minecraft 1.14
Other
194 stars 71 forks source link

cloth-config dependency support #204

Closed MattSturgeon closed 1 year ago

MattSturgeon commented 1 year ago

Following on from my initial draft PR #195, here's a subset of the changes focused on adding dependency support to cloth config.

I identified the main cause of complexity in the draft PR was auto-generating human readable descriptions of potentially complex dependencies. In hindsight, the result isn't worth the added complexity, instead it is recommended that users write their own dependency descriptions in tooltips or labels.

This PR does add a simple "Dependencies not met" line to the tooltip when isEnabled() is false. This could be removed/reformatted/reworded depending on your preference.

Currently, the Dependency may be checked multiple times per frame. If this proves performance intenseive, it may make sense to instead check once-per-tick on the main thread and store the result in an AtomicBoolean to reference from the render thread. implemented

I plan to add an implementation for autoconfig in a follow up PR, based on what I have currently in the initial draft, but ideally this should be merged first - or at least the design agreed upon.

Fixes #26

MattSturgeon commented 1 year ago

@shedaniel do you think you'll get time to review this soon? I'm open to feedback and discussing the overall design if you have a different approach in mind.

shedaniel commented 1 year ago

Thank you for this PR and sorry that I have completely forgotten about this, please continue to notify me if I forget in the future. I will take a quick look later.

MattSturgeon commented 1 year ago

@shedaniel I think I've addressed your points.


In 69d3590d2fa93476f1c9309b2ba93a398ca7eff3 I took a different approach to ValueHolder, splitting it into NullableValueHolder and NotNullValueHolder.

All Config Entries implement NullableValueHolder, a select few config entries who know thieir value is not null override the implementation with NotNullValueHolder. I haven't gone through and exhaustively checked which classes can be NotNull yet, I just picked out some of the important examples.

If you don't like this approach I can revert that commit, but it will make some otherwise trivial dependencies rather verbose:

builder.setRequirement(Requirement.any(
                        () -> Optional.ofNullable(dependency.getValue())
                                .map(value -> value < -70)
                                .orElseThrow(),
                        () -> Optional.ofNullable(dependency.getValue())
                                .map(value -> value > 70)
                                .orElseThrow()))
// vs
builder.setRequirement(Requirement.any(
                        () -> dependency.getValue() < -70,
                        () -> dependency.getValue() > 70))

I also went through DynamicEntryListWidget and optimized out a bunch of calls to visibleChildren() (etc) as requested.


I've marked the new stuff as Experimental, but it's getting late here so I've probably missed some bits. Did you want anything marked as Internal too/instead?

MattSturgeon commented 1 year ago

Fixed your latest points. Also found and fixed a bug where a hidden boolean toggle entry was still handling click events: aa22d5e7c82dba4375986ef4440ee5de35acccf0

shedaniel commented 1 year ago

Also I am okay for explicit @Nullable, just no for explicit @NotNulls, it is fine to keep the @Nullable

MattSturgeon commented 1 year ago

Great, thanks for all your help and guidance. 🎉

Would you like the PR to target a different branch or stick with v8?

Enjoy your trip!