Test#1
Merged
Merged
Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ht.userdev.gradle.plugin to v2.0.0-SNAPSHOT (#3493) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…3508) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: dordsor21 <dordsor21@gmail.com>
…3514) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: SirYwell <hannesgreule@outlook.de> Co-authored-by: Maddy Miller <git@madelinemiller.dev>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…3521) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (51)
📝 WalkthroughWalkthroughThis PR refactors tree generation into a new record-based API, updates Gradle and dependencies, and enhances schematic saving with caption-based messaging. Tree generation shifts from an enum-based approach to a ChangesTree Generation API Refactoring
Gradle and Build Tooling Updates
Schematic Saving and Memory Monitoring Enhancements
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoImplement translatable schematic commands and tree type registry system
WalkthroughsDescription• Implement translatable schematic commands with caption-based messages • Add TreeType registry and generation support across all adapters • Migrate from deprecated TreeGenerator.TreeType to new TreeType class • Fix memory warning cooldown and threshold detection in FAWE • Update dependencies and Gradle to latest versions Diagramflowchart LR
A["SchematicCommands"] -->|uses Captions| B["TreeType Registry"]
C["EditSession"] -->|makeForest methods| B
D["BukkitImplAdapter"] -->|generateTree| B
E["TreePlanter Tool"] -->|uses| B
F["Gradle Updates"] -->|dependencies| G["Build System"]
File Changes1. worldedit-core/src/main/java/com/sk89q/worldedit/command/SchematicCommands.java
|
Code Review by Qodo
1. gradlew.bat fallthrough on error
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+54
to
57
| "%COMSPEC%" /c exit 1 | ||
|
|
||
| :findJavaFromJavaHome | ||
| set JAVA_HOME=%JAVA_HOME:"=% |
There was a problem hiding this comment.
1. Gradlew.bat fallthrough on error 🐞 Bug ☼ Reliability
In gradlew.bat, the missing/invalid JAVA_HOME paths run "%COMSPEC%" /c exit 1 but do not exit the current batch script, so execution falls through into later labels and can attempt to run Gradle with an invalid %JAVA_EXE% (e.g., /bin/java.exe). This can mask the real problem and produce confusing follow-on failures.
Agent Prompt
### Issue description
`gradlew.bat` uses `"%COMSPEC%" /c exit 1` in error branches for missing/invalid Java, but that only exits the spawned `cmd.exe` process and does not reliably terminate the current batch script. The script then continues into subsequent labels and can reach `:execute` with a bad `%JAVA_EXE%`.
### Issue Context
We need the batch script to terminate immediately with a non-zero exit code when Java cannot be found.
### Fix Focus Areas
- gradlew.bat[41-82]
### Suggested fix
Replace both `"%COMSPEC%" /c exit 1` occurrences in the error paths with `exit /b 1` (or `goto :exitWithErrorLevel` after setting `ERRORLEVEL=1`). Ensure the script cannot fall through to `:execute` when Java is missing/invalid.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+937
to
+949
| // Trees | ||
| HolderLookup.RegistryLookup<PlacedFeature> placedFeatureRegistry = server.registryAccess().lookupOrThrow(Registries.PLACED_FEATURE); | ||
| placedFeatureRegistry.listElements() | ||
| .filter(feature -> { | ||
| var underlyingFeature = feature.value().feature().value().feature(); | ||
| return underlyingFeature instanceof TreeFeature || underlyingFeature instanceof CoralTreeFeature; | ||
| }) | ||
| .forEach(feature -> { | ||
| String key = feature.key().toString(); | ||
| if (TreeType.REGISTRY.get(key) == null) { | ||
| TreeType.REGISTRY.register(key, new TreeType(key)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
2. Unparseable treetype ids 🐞 Bug ≡ Correctness
PaperweightAdapter.initializeRegistries() registers TreeType IDs using feature.key().toString(), but tree generation later assumes treeType.id() is a valid namespace:path and passes it to ResourceLocation.tryParse(...). If the stored ID is not parseable, tree generation can throw (via getOrThrow) or fail due to null keys.
Agent Prompt
### Issue description
Tree types are registered with `String key = feature.key().toString()`, but later code uses `ResourceLocation.tryParse(treeType.id())` expecting a plain `namespace:path`. If `feature.key().toString()` includes extra formatting, parsing can return null and tree generation will error.
### Issue Context
Tree IDs must round-trip between registry population and later lookup.
### Fix Focus Areas
- worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/ext/fawe/v1_21_R1/PaperweightAdapter.java[937-989]
- worldedit-bukkit/adapters/adapter-1_20_4/src/main/java/com/sk89q/worldedit/bukkit/adapter/ext/fawe/v1_20_R3/PaperweightAdapter.java[925-938]
### Suggested fix
When iterating placed features, derive the tree ID from the actual `ResourceLocation` (e.g., `feature.key().location().toString()` if available, or equivalent API) rather than `toString()` of the key object.
Also add a defensive guard in generateTree: if `ResourceLocation.tryParse(treeType.id()) == null`, return false (and optionally log/debug). Apply consistently across affected adapters.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+694
to
+709
| PlacedFeature placedFeature = serverLevel | ||
| .registryAccess() | ||
| .registryOrThrow(Registries.PLACED_FEATURE) | ||
| .get(ResourceLocation.tryParse(treeType.id())); | ||
|
|
||
| FaweBlockStateListPopulator populator = new FaweBlockStateListPopulator(serverLevel); | ||
| List<CraftBlockState> placed = TaskManager.taskManager().sync(() -> { | ||
| preCaptureStates(serverLevel); | ||
| try { | ||
| if (!placedFeature.place( | ||
| populator, | ||
| generator, | ||
| serverLevel.random, | ||
| new BlockPos(pt.x(), pt.y(), pt.z()) | ||
| )) { | ||
| return null; |
There was a problem hiding this comment.
3. Generatetree null dereference 🐞 Bug ☼ Reliability
Several PaperweightFaweAdapter.generateTree() implementations fetch a PlacedFeature with registry.get(...) and then unconditionally call placedFeature.place(...). If the registry lookup returns null (missing key or parse failure), this will throw a NullPointerException instead of returning false.
Agent Prompt
### Issue description
`generateTree()` obtains a `PlacedFeature` from the registry and immediately calls `.place(...)` without checking for null, which can crash the operation.
### Issue Context
Other feature generation code paths in the project use `k != null && k.place(...)` and return `false` when missing.
### Fix Focus Areas
- worldedit-bukkit/adapters/adapter-1_20_4/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R3/PaperweightFaweAdapter.java[684-720]
- worldedit-bukkit/adapters/adapter-1_20_2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R2/PaperweightFaweAdapter.java[685-721]
- worldedit-bukkit/adapters/adapter-1_20_5/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_20_R4/PaperweightFaweAdapter.java[698-734]
- worldedit-bukkit/adapters/adapter-1_21/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v1_21_R1/PaperweightFaweAdapter.java[699-735]
### Suggested fix
After parsing the ResourceLocation and fetching from the registry, return `false` if the parsed key is null or the registry lookup returns null. Only call `.place(...)` when non-null, mirroring how `generateFeature()` handles missing entries.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores