Various changes to StructureTemplates, 1.20.5 branch. #13
Conversation
solonovamax
changed the title
There was a problem hiding this comment.
Alright, I've gone through and commented this version of the PR - the content applies to all the other ones. I would also like to mention that I will not merge any of them besides this one, 1.20 and 1.21 - we don't support the others anymore.
Overall, please please take a look at our Contributing Guidelines again and make sure your contribution follows the standards set therein.
As for the features, I'd be willing to merge the template parsing changes once they're cleaned up - for the rest, refer to my comments
Cheers
src/main/java/io/wispforest/lavender/book/LavenderClientStorage.java
Outdated
Show resolved
Hide resolved
src/main/java/io/wispforest/lavender/structure/BlockStatePredicate.java
Outdated
Show resolved
Hide resolved
src/main/java/io/wispforest/lavender/structure/BlockStatePredicate.java
Outdated
Show resolved
Hide resolved
src/main/java/io/wispforest/lavender/structure/StructureTemplate.java
Outdated
Show resolved
Hide resolved
| BlockStatePredicate predicate; | ||
| if (keys.containsKey(key)) { | ||
| predicate = keys.get(key); | ||
| private record StructureTemplateRenderView(@NotNull World world, @NotNull StructureTemplate template) implements BlockRenderView { |
There was a problem hiding this comment.
Why is this a record now? It was doing perfectly fine as a local class
There was a problem hiding this comment.
because intellij was yelling at me saying to make it a record lol
There was a problem hiding this comment.
Why is this a record now? It was doing perfectly fine as a local class
I pulled it out as an inner class as I find local classes to be a bit of a codesmell.
Would you like me to revert this?
src/main/java/io/wispforest/lavender/structure/StructureTemplate.java
Outdated
Show resolved
Hide resolved
src/main/java/io/wispforest/lavender/structure/StructureTemplate.java
Outdated
Show resolved
Hide resolved
src/main/java/io/wispforest/lavender/mixin/MinecraftClientMixin.java
Outdated
Show resolved
Hide resolved
| if (state.getBlock() != predicate.blockState().getBlock()) return Result.NO_MATCH; | ||
| @NotNull | ||
| @Override | ||
| public Iterator<Pair<BlockPos, BlockStatePredicate>> iterator() { |
There was a problem hiding this comment.
What precisely is the use case for this over the iteration methods? Having the entire iterator implementation below (which once again lays its fields out in a manner inconsistent with the rest of the codebase) seems like code I'd rather not have to maintain if there's not a good application
There was a problem hiding this comment.
my specific usecase was:
iterate over all the blocks in a structure, exiting immediately when it finds the first block that matches some conditions.
the reason forEachPredicate wasn't appropriate for me, was it would not exit after the first match. what I wanted was something that a Stream is perfect for, for which implementing Iterable was needed.
however, with predicates being exposed, I can implement that in my own codebase. but, for posterity I contributed both changes.
There was a problem hiding this comment.
Leaving this as unresolved while I wait for you to reply.
… in keys Allow for arrays of strings in the keys of a structure template. All the elements are parsed the same as they previously were, this just allows for defining a list of possible blocks. Also clean up code a decent amount to make it a tad more maintainable. Signed-off-by: solonovamax <[email protected]>
- Implement Iterable<Pair<BlockPos, BlockStatePredicate>> for StructureTemplate - Convert all fields to getters and introduce getters for more fields Signed-off-by: solonovamax <[email protected]>
solonovamax
force-pushed
the
structure-entries-list/1.20.5
branch
from
6f6ee8c to
8ee713a
Compare
Changed several things to do with
StructureTemplates: