-
Notifications
You must be signed in to change notification settings - Fork 500
Add an event to modify dimension environment attributes #5098
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
base: 26.1
Are you sure you want to change the base?
Conversation
fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/api/dimension/v1/ModificationPhase.java
Outdated
Show resolved
Hide resolved
...-v1/src/main/java/net/fabricmc/fabric/mixin/dimension/modification/MinecraftServerMixin.java
Outdated
Show resolved
Hide resolved
...ions-v1/src/main/java/net/fabricmc/fabric/api/dimension/v1/DimensionModificationContext.java
Outdated
Show resolved
Hide resolved
maityyy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a useful API that we should definitely have, but I'm sure all we need is something like EnchantmentEvents or LootTableEvents, copying the Biome Modification API design is completely unnecessary, confuses users and inflates the code base too much
b1c2eee to
2bded09
Compare
2bded09 to
6e1c7ca
Compare
|
Thanks a lot for the feedback @maityyy, I fully agreed with your points and have almost nothing to add really. I've gone back and remade the whole thing from the ground up using events. I have to note that modifying attributes using Therefore, I used I've updated the PR's title and description accordingly. |
| stopwatch.stop(); | ||
|
|
||
| if (dimensionsProcessed > 0) { | ||
| LOGGER.info("Applied modifications to {} of {} dimensions in {}", dimensionsChanged, dimensionsProcessed, stopwatch); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the stopwatch needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not, it's a leftover from the PR author's initial attempt to copy the Biome Modification API design. BMAPI uses Stopwatch probably because of the logic of sorting modifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I imagined so. I dont think its really that useful in biome modficiation either tbh.
| if (dimensionsModified) { | ||
| throw new IllegalStateException("Dimensions in this dynamic registries instance have already been modified"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this the only use for the dimensionsModified boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! This is meant to prevent any accidental double-modification, but that should never occur
...fabricmc/fabric/mixin/dimension/modification/RegistryAccessImmutableRegistryAccessMixin.java
Outdated
Show resolved
Hide resolved
fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/impl/dimension/FabricDimensionsImpl.java
Outdated
Show resolved
Hide resolved
fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/impl/dimension/FabricDimensionsImpl.java
Outdated
Show resolved
Hide resolved
fabric-dimensions-v1/src/main/resources/fabric-dimensions-v1.accesswidener
Outdated
Show resolved
Hide resolved
| ServerLevel overworld = server.getLevel(Level.OVERWORLD); | ||
| overworld.setDayTime(6000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this needed to get the color? If so a game test would likely be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's needed. I don't understand how the MODIFY_ATTRIBUTES can be fired in the game test environment though.
...src/main/java/net/fabricmc/fabric/impl/dimension/modification/DimensionModificationImpl.java
Outdated
Show resolved
Hide resolved
| // Build a list of all dimension keys in ascending order of their raw-id to get a consistent result in case | ||
| // someone does something stupid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stupid such as? Doing this sorting will likely hide mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from previous iteration. I figured I'd leave this comment from BMAPI since it makes debugging easier if someone messes up badly.
fabric-dimensions-v1/src/main/java/net/fabricmc/fabric/api/dimension/v1/DimensionEvents.java
Show resolved
Hide resolved
...src/main/java/net/fabricmc/fabric/impl/dimension/modification/DimensionModificationImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (dimensionsProcessed > 0) { | ||
| LOGGER.info("Applied modifications to {} of {} dimensions in {}", dimensionsChanged, dimensionsProcessed, stopwatch); | ||
| LOGGER.info("Applied modifications to {} of {} dimensions", dimensionsChanged, dimensionsProcessed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging is not necessary, please remove
This pull request introduces a new API to facilitate the modification of dimension environment attributes, which are natively immutable.
Rationale
Since the introduction of environment attributes, several use cases have emerged requiring modifications at the dimension level. Currently, applying an attribute across an entire dimension requires iterating through and modifying every individual biome within that dimension using the Biomes Modification API.
This event provides a direct interface for interacting with dimensions attributes. Furthermore, it allows datapack and modpack creators to maintain granular control, as dimension-level modifications are designed with lower priority than biome-specific definitions during attribute evaluation in vanilla.
Usage Example