Coalesce Base Predicate counts + information into record and make MultiPredicate have counts - #5342
Coalesce Base Predicate counts + information into record and make MultiPredicate have counts#5342ghzdude wants to merge 26 commits into
Conversation
644713d to
da9a8e9
Compare
add set settings copy settings
count with new interface instead of PredicateSettings organize methods in MultiPredicate
…ed to the MultiPredicate instead of its children javadoc
add builder pattern generic to SettingsHolder
improve nonevalid in xor
da9a8e9 to
1dcbd26
Compare
| } | ||
|
|
||
| default int getMaxCount() { | ||
| return getSettings().maxCount(); |
There was a problem hiding this comment.
All of these directly reference getSettings() but settings is nullable, so these should have guards for that then
| if (testGlobalMin(ctx)) return true; | ||
| boolean passed = testGlobalMin(ctx); | ||
| if (this.settings != null) { | ||
| passed &= SettingsHolder.super.testGlobalMin(ctx.getGlobalCount(this)); |
There was a problem hiding this comment.
This specific SettingsHolder.super.testGlobalMin(int count) doesnt' seem to be tested for any children in the recursive steps, only in the top level predicates (e.g. the AndPredicate etc' testGlobalMin doesnt' call anything that calls testGlobalMin(int count) on the children)
| return passedPredicate.testSliceMax(context); | ||
| boolean passed = passedPredicate.testGlobalMax(context); | ||
| if (this.settings != null) { | ||
| passed &= this.testGlobalMax(context.incrementGlobalCount(this)); |
There was a problem hiding this comment.
This only increments the count itself, not of ancestors, so a predicate whose count requirements are satisfied through only its children will never match
| @Override | ||
| public List<Component> getRecipeViewerTooltips(MultiPredicate root) { | ||
| List<Component> tooltips = new ArrayList<>(this.getAdditionalTooltips()); | ||
| List<Component> tooltips = new ArrayList<>(); |
There was a problem hiding this comment.
Why was the getAdditionalTooltips call removed?
| public MultiPredicate withSettings(UnaryOperator<PredicateSettings> configurator) { | ||
| return copyWith(p -> { | ||
| // is this (the current reference being copied) have their settings set to null? | ||
| if (this.settings == null) { |
There was a problem hiding this comment.
in this case with the XorPredicate, nothing actually recomputes isNoneValid after this happened and its children have a potentially now valid predicate
| @CheckReturnValue | ||
| public MultiPredicate setMaxSliceCount(int max) { | ||
| return mutatedCopy(p -> p.setMaxSliceCount(max)); | ||
| public void updateSettings(UnaryOperator<PredicateSettings> configurator, boolean shouldCreate) { |
There was a problem hiding this comment.
This means any non-single recursive (settings = null) predicate who gets any e.g. .setMinCount() applied to it actually no-ops right?
|
|
||
| @CheckReturnValue | ||
| public MultiPredicate setMaxGlobalLimited(int max) { | ||
| return this.setMaxCount(max); |
There was a problem hiding this comment.
because e.g. setMinCount and setMaxCount now just call SettingsHolder.setMinCount() which doesn't make a mutated copy anymore, doing stuff like Predicates.AIR.setMinCount(4) mutates the global air predicate. This is why we did all those mutatedCopy things
| if (multiPredicate.isSingle() || multiPredicate.isType(type)) { | ||
| List<BasePredicate> predicates, List<MultiPredicate> children, | ||
| boolean equalSettings) { | ||
| if (multiPredicate.isSingle() || (multiPredicate.isType(type) && equalSettings)) { |
There was a problem hiding this comment.
I don't think we wanna merge on equalSettings, since then blocks(a,b).setMax(2).or(blocks(c,d).setMax(2)) would collapse into the equivalent of blocks(a,b,c,d).setMax(2) which is bad, since the original semantically should match aacc
| @CheckReturnValue | ||
| public MultiPredicate setMinLayerLimited(int min, int previewCount) { | ||
| return this.setMinSliceCount(min).setPreviewCount(previewCount); | ||
| /// Sets the setting of this multipredicate to its children, then makes {@code this.settings} null |
|
Edit to add: review assisted by opus |
What
Multipredicates should now be able to be treated like base predicates for testing min/max counts
Implementation Details
Added record
PredicateSettingswhich combines several of BasePredicate's fieldsMultiPredicate also has a settings field, though it is nullable unlike in BasePredicate
For MultiPredicates, if
settings == nullthey behave exactly as before.When
settingsis nonnull, it also gets checked along with children predicatesboth must be
trueto passLang has also been introduced for the to-be-used recipe view tooltips
AI Usage
Outcome
you should be able to do
a.or(b).setMinCount(4)to mean "4 combined of either predicates" nowHow Was This Tested
ran client, only check the assembly line which formed as expected