Make ^ support nbt (properly) by QuImUfu · Pull Request #2734 · EngineHub/WorldEdit

QuImUfu · 2025-03-14T23:30:10Z

My implementation of #2733,
It adds ^{=nbt} support, integrated in current ^ support to allow replacing/keeping arbitrary parts of a block.

Additionally, adds {nbt} support for merging NBT tags into existing ones.

Examples:

//replace chest ^barrel will keep orientation and NBT intact and just replace the block type, keeping e.g. contents as-is.
//replace chest ^[facing=north] will make all chest face north, keeping their NBT intact.
//replace chest,barrel ^{=LootTable:"minecraft:chests/simple_dungeon"} will make both, chests and barrels contain dungeon loot, while removing any potential other NBT.
//set ^{=} deletes nbt from selection. (does not work for LootTable tags due to a bug in vanilla MC I can't link here or find again because mojangs bug tracker is completely broken)
//replace decorated_pot ^{LootTable:"minecraft:chests/simple_dungeon"} will make pots contain dungeon loot while keeping their pattern.
...

me4502

Thanks for this. I've left a comment around API breakages. I haven't reviewed the actual parser itself as the diff is broken currently by the rename.

Also as this is adding behaviour, this should be targeting master, not 7.3.x.

me4502 · 2025-03-16T08:24:17Z

+import java.util.stream.Stream;
+
+
+public class PartiallyApplyingPatternParser extends InputParser<Pattern> {


Renaming this class would be an API break, so it'd need to retain its original name, or duplicate it and deprecate the old one.

What is the rule for API breaks/changes? Am I allowed to change behaviour in a potentially breaking way (this is) and still maintain the same name/API? Or do I need to provide a fully backwards-compatible implementation?

I created a deprecated legacy version, that should behave like before. Do I need to do the same for the two Patterns that I changed?
Is there some documentation on what needs to stay API compatible?

Typically as long as it's binary compatible and works vaguely as expected it should be fine. Slight behaviour changes can happen IMO.

me4502 · 2025-03-16T08:25:33Z

    @Override
    public BaseBlock applyBlock(BlockVector3 position) {
-        BlockState oldBlock = getExtent().getBlock(position);
+        BaseBlock oldBlock = getExtent().getFullBlock(position);


Have you tested how well this performs? This fundamentally means the pattern is doing a lot more work per-block.

Also, have you tested how this behaves when set to another block with a block entity of a different type, with conflicting data? Eg, change a chest with items to a jukebox and make sure it doesn't explode?

It does a lot more (in some cases unavoidable) work. I did not do large scale testing where performance issues would become apparent. I will think about how we can avoid incurring a performance penalty (if there is one, I'll test that) for blocks that can't have NBT or when NBT is overwritten anyway.

If it breaks something, it can already break with the vanilla /data command. But it and the data command, in fact, are able to break a lot of (as far as I have tested only non-vanilla) blocks, and AFAICT, there is not much we could do about it. We could maintain a gigantic database of sanitizers, one for every block with tile entity in every mod, plead to mod devs to sanitize their NBT properly, or we could prevent the game from flushing to disk until after the command finished, to prevent game-cashing NBT from corrupting worlds, at the expense of possibly prohibitive RAM use.

I wasn't able to detect any performance impact, which honestly surprised me. Not sure if my testing was flawed.
The performance impact seems, at least on my PC, to be completely drowned by IO for huge edits, and be undetectably small for smaller ones.
If you manage to find a scenario in which my changes have a significant performance impact, I'd be happy to take another look.

The case where this would likely have the most impact is when the selection has a large amount of complex NBT data in it, as the change basically leads to reading that data from the world.

Although tbh AFAICT this change here is mostly a bugfix anyway, so if it's performant it might be fine to PR separately to version/7.4.x

I tried fitting as many barrels with items in the loaded chunks as my heap size allows (16GiB) to avoid the IO bottlenecks I experienced in my previous testing, and replacing them with and without the changes in this PR. And while the time till the replaced ... blocks message didn't meaningfully change, the time till the game became responsive again did significantly increase with these changes (between 30% and 50%).

I see a similar slowdown merging NBT vs. setting NBT. So it seems that reading NBT for each block indeed can cause a significant difference, that will likely increase with more complex NBT (e.g. complex items in the barrels).

I will try to include an optimisation that makes the respective patterns skip reading & writing NBT if that is unnecessary. This should at least allow users experiencing performance issues to just specify {=} and get the previous performance back, at the cost of the previous behaviour.

octylFractal · 2025-03-16T21:48:05Z

I have concerns about the exact syntax here, I don't think we should allow ^{} to be deletion / require a prefix comma -- this is inconsistent with the existing function.

As it stands, ^ is purely overwrite/additive. ^foo means "set block type to foo keeping all applicable state entries", ^[x=y] means "set the block state to the previous state with x set to y". I think it should remain that way for NBT as well, which would imply that ^{} is essentially a no-op and should be illegal syntax, as ^[] is today. If we want to add a variant that clears existing state, I think it should use a different prefix, or perhaps an extended one such as ^= ("equals" to imply removal of unwritten state).

QuImUfu · 2025-03-16T23:36:21Z

While I understand you concern, I have some concerns with merge only for ^:

The current syntax allows for simply including {} to sidestep any NBT handling and (if implemented correctly, I probably will have to fix that) prevent any performance penalty.

If we do merge only, we need to add a different way to have ^[..=..] and ^.. keep (with related performance penalty) or drop NBT.

I'd suggest following to make it more consistent, without introducing a new prefix:
Changing {,..} to just {..} and {..} to {=..} or ={..} (the second being slightly confusing in combination with other overrides) and keeping merging as default this way. But that'd still include new syntax for ^ only applicable to NBT (we could also add ^[=..] but that would be a mostly useless feature just for consistencies’ sake).

If you have a different idea on how that could be implemented more consistently, I'm very open to suggestions.

QuImUfu · 2025-03-21T13:29:26Z

I have changed it to merge by default and set with {=...}.

madebyisaacr · 2026-07-28T04:55:46Z

Does this PR also preserve sign and hanging sign text? If so, it will close #2546

QuImUfu · 2026-07-31T14:13:29Z

Does this PR also preserve sign and hanging sign text? If so, it will close #2546

Yes. Along with preserving chest/barrel contents, pot contents ...

Updated the PR to current master.

QuImUfu · 2026-08-03T22:20:06Z

Updated the MR to fix an issue with the command mentioned in #2998

I am not quite confident in the changes in StringUtils.parseListInQuotes, because I am unsure of the original intent of that method.

me4502

Sorry it's been so long – I missed the initial changes you made after the reviews, and then with the uptick in MC update frequency/etc I haven't had a chance to look at the PR backlog outside of ones I was actively tracking.

I've replied to the comments, otherwise I'll let @octylFractal review the actual parser behaviour here. I'm a little apprehensive about the complexity of this parser at this point

me4502 · 2026-08-04T11:32:15Z

    @Override
    public BaseBlock applyBlock(BlockVector3 position) {
-        BlockState oldBlock = getExtent().getBlock(position);
+        BaseBlock oldBlock = getExtent().getFullBlock(position);


The case where this would likely have the most impact is when the selection has a large amount of complex NBT data in it, as the change basically leads to reading that data from the world.

Although tbh AFAICT this change here is mostly a bugfix anyway, so if it's performant it might be fine to PR separately to version/7.4.x

me4502 · 2026-08-04T11:34:06Z

+import java.util.stream.Stream;
+
+
+public class PartiallyApplyingPatternParser extends InputParser<Pattern> {


Typically as long as it's binary compatible and works vaguely as expected it should be fine. Slight behaviour changes can happen IMO.

QuImUfu · 2026-08-04T22:11:05Z

I added a Test to increase my confidence in the implementation of the PartiallyApplyingPatternParser, however, I had to add a couple of public getters to the Patterns for validation, and I am not sure if that is an acceptable in this project. If it isn't, I am open to any suggestions on how to do it better.

Also, can I add a component test spanning the entire parsing logic? I am unsure if the Parsers interact correctly for nested/mixed Patterns.
Or are only Unit Tests permitted?

QuImUfu force-pushed the nbt-aware-partial-patterns branch from a572827 to 9c974cf Compare March 15, 2025 08:34

QuImUfu marked this pull request as ready for review March 15, 2025 08:53

QuImUfu requested a review from a team as a code owner March 15, 2025 08:53

me4502 requested changes Mar 16, 2025

View reviewed changes

QuImUfu changed the base branch from version/7.3.x to master March 16, 2025 20:06

QuImUfu force-pushed the nbt-aware-partial-patterns branch from 9c974cf to 4f58081 Compare March 21, 2025 12:51

QuImUfu requested review from me4502 and octylFractal April 5, 2025 07:15

QuImUfu force-pushed the nbt-aware-partial-patterns branch from 4f58081 to e2f2e19 Compare July 31, 2026 14:08

QuImUfu force-pushed the nbt-aware-partial-patterns branch from e2f2e19 to 5f39ada Compare August 3, 2026 22:04

QuImUfu force-pushed the nbt-aware-partial-patterns branch from a382de1 to cda2816 Compare August 4, 2026 05:37

me4502 reviewed Aug 4, 2026

View reviewed changes

make ^ support nbt (properly)

1df5bb2

QuImUfu force-pushed the nbt-aware-partial-patterns branch 2 times, most recently from 68cef48 to cce709b Compare August 4, 2026 21:54

Test for PartiallyApplyingPatternParser

a7abc4f

QuImUfu force-pushed the nbt-aware-partial-patterns branch from cce709b to a7abc4f Compare August 4, 2026 21:56

		import java.util.stream.Stream;


		public class PartiallyApplyingPatternParser extends InputParser<Pattern> {

Uh oh!

Uh oh!

Conversation

QuImUfu commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

me4502 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octylFractal commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuImUfu commented Mar 16, 2025

Uh oh!

QuImUfu commented Mar 21, 2025

Uh oh!

madebyisaacr commented Jul 28, 2026

Uh oh!

QuImUfu commented Jul 31, 2026

Uh oh!

QuImUfu commented Aug 3, 2026

Uh oh!

me4502 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuImUfu commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

QuImUfu commented Mar 14, 2025 •

edited

Loading

octylFractal commented Mar 16, 2025 •

edited

Loading