Skip to content

Conversation

@Argmaster
Copy link
Collaborator

@Argmaster Argmaster commented Mar 2, 2025

This pull request introduces some of the commands available in very popular minecraft mod called WorldEdit.
List of WorldEdit commands can be found here
For best user experience I decided to take approach of implementing those commands as close to original as possible.

CHANGELOG:

  • /pos1
  • /pos2
  • /copy
  • /paste
  • /blueprint save
  • /blueprint load
  • /blueprint delete
  • /blueprint list
  • binary storage format
  • wireframe outline of selected region

Even if Cubyz engine is not ready for large amounts of block updates I think that players don't really care and will be happy to have those features despite potential lag spikes.

I do not plan adding brushes in this PR, it will be waaay too much code at once. For that reason rotate and flip are also considered optional, maybe they can be added in a basic version where they just move blocks around but don't update block data to eg. flip stairs. Pattern expressions are also outside of the scope of this PR, for now commands that take patterns will be only able to take one string block ID.

Related to: #1214

@Argmaster
Copy link
Collaborator Author

Oh and also this PR adds schematic storage format as part of //blueprint save

@ikabod-kee
Copy link
Collaborator

If possible, can you add a wand item? It doesn't need a fancy texture

@Argmaster
Copy link
Collaborator Author

If possible, can you add a wand item? It doesn't need a fancy texture

Sure, but in separate PR - we are already near 500 lines - I think that I should also leave rest of the features listed (maybe except schematic loading) for separate PR. @IntegratedQuantum won't be happy about reviewing my spagetti 🥲

@IntegratedQuantum
Copy link
Member

At a glance I already see a few issues:

  • Storing blueprints as zon is just about the worst thing you could do for file size, please store binary data + at least deflate compression.
  • Shouldn't blueprints also save the blockdata, and not just the block type? Also the use of a palette seems weird, looks like it would strictly makes memory usage and performance worse here.
  • Saving blueprints on the server is probably not the right way to go about it.
  • Overwriting the command id should not be needed, you can just use zig's @"" syntax to create any name you want as an identifier.
  • Please don't introduce a new command prefix // without any good reason. There should be only one way to write commands everything else is just needlessly confusing.

@Argmaster
Copy link
Collaborator Author

Argmaster commented Mar 2, 2025

At a glance I already see a few issues:

  • Storing blueprints as zon is just about the worst thing you could do for file size, please store binary data + at least deflate compression.

I agree, but this is a quick way to do it and I want to support a human readable format for sake of being able to sit and scribble something in notepad if needed. As you already said, Cubyz is not ready for large amounts of block updates, so let's maybe avoid premature optimalization for sake of having features, what do you think?

Regardless of what I just said, I agree that this should not be a primary storage format, I can and will work on a binary format, but I don't want to do it in this PR, IMO this is beyond providing simple WorldEdit functionality, so it is beyond of my goals right now.

On top of that I would like to also at least partially support imports from minecraft schematic format to a reasonable extent with some limited translation table, but that is a material for yet another PR.

  • Shouldn't blueprints also save the blockdata, and not just the block type?

I think they are doing that, the palette stores only block type, but block array stores both, unless there is a bug in the code.

Also the use of a palette seems weird, looks like it would strictly makes memory usage and performance worse here.

You have to translate from block palette that was present in world you exported from to block palette of world you are importing into, at least because they could have been created on different versions of the game, so there must be a palette storage. Since most of the time you will not be using whole palette, there is also no point in storing whole palette, which can have up to 65k entries. Therefore palette is created with blocks that are necessary and reassigning IDs ensures there are no gaps in ID list.

For rotations and flips you will have to translate it back to world palette regardless of what palette you use for storage, as rotations and flips must also work on schematics you load from drive.

  • Saving blueprints on the server is probably not the right way to go about it.

This is what WorldEdit does, sure, we can do something different but then you have to transfer schematic to client and that is more complicated for limited benefit. Users of those commands are expected to have admin rights, thus also access to the server.

  • Overwriting the command id should not be needed, you can just use zig's @"" syntax to create any name you want as an identifier.

That's great, I didn't know that!

  • Please don't introduce a new command prefix // without any good reason. There should be only one way to write commands everything else is just needlessly confusing.

I guess this is it for sticking with WorldEdit design. I know, It's Cubyz, not minecraft, but being unique is not always an advantage, especially from user experience it is usually better to follow standards that were established before.

Anyway, that extra slash shouldn't be a big deal so I will get rid of it, I just had to talk back 😝

Copy link
Contributor

@OneAvargeCoder193 OneAvargeCoder193 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a couple issues with the way that you store the selected blocks

@Argmaster
Copy link
Collaborator Author

@OneAvargeCoder193 I moved pos1, pos2 and clipboard to user data.

@IntegratedQuantum
Copy link
Member

I agree, but this is a quick way to do it and I want to support a human readable format for sake of being able to sit and scribble something in notepad if needed. As you already said, Cubyz is not ready for large amounts of block updates, so let's maybe avoid premature optimalization for sake of having features, what do you think?

The quick way it is going to screw us over in the long run. And it isn't even that much quicker than the alternative, instead of doing zon.get, you just read stuff at an offset. And utils.Compression makes it easy to compress the file data.
A binary format will be necessary at some point, and if we don't add it now, then we'll need to add conversion code.
And in comparison a binary format can be easily made future proof by adding a version number to the header.
And seriously, no one is going to scribble their structures in a text editor, when they could just build it in the game.

Also Cubyz may not be ready for large block updates, but if it's there, then I'm sure that @OneAvargeCoder193 will be tempted to use it for structure generation.

On top of that I would like to also at least partially support imports from minecraft schematic format

That should be a third-party conversion tool.

I think they are doing that, the palette stores only block type, but block array stores both, unless there is a bug in the code.

If it does that, then please use the Block struct for it for better readability, right now it's just a u32.

You have to translate from block palette that was present in world you exported from to block palette of world you are importing into, at least because they could have been created on different versions of the game

Ah, if it's only for storing the file, then it should only be used once when loading it to translate the block ids. Doing this translation every time a structure is pasted, is just wasteful.

This is what WorldEdit does, sure, we can do something different but then you have to transfer schematic to client and that is more complicated for limited benefit. Users of those commands are expected to have admin rights, thus also access to the server.

I was thinking, maybe a player would want to take a structure they built in another world and paste it one some multiplayer server or something like that. But it's fine with me if you don't want to support that.

@Argmaster
Copy link
Collaborator Author

image

house.zip

@Argmaster
Copy link
Collaborator Author

There is literally 0 error handling while loading tho, so there is still a bit to be done before making it ready for review

@IntegratedQuantum
Copy link
Member

Before you put any more work into it, I'd like to do some improvements to how other binary formats are handled, to improve error handling, among other things.

@Argmaster
Copy link
Collaborator Author

Before you put any more work into it, I'd like to do some improvements to how other binary formats are handled, to improve error handling, among other things.

How is that a bottle neck for this PR?

@IntegratedQuantum
Copy link
Member

How is that a bottle neck for this PR?

Well you did say you want to implement more error handling, this is me telling that you should wait.

@Argmaster
Copy link
Collaborator Author

Well, I ment to add some logging / return errors instead of panicking on everything, I did not anticipate it would stall this PR.

@IntegratedQuantum
Copy link
Member

Well it won't stall the PR, but since this is not going to finish review in the next weeks, you might as well wait with it, until I implement better options.

@Argmaster
Copy link
Collaborator Author

this is not going to finish review in the next weeks

Good to know

Copy link
Member

@IntegratedQuantum IntegratedQuantum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this and found one small issue with blocks that require support: #1217
However, please do not add a fix to this PR, since it is non-trivial to fix and I would like to get this merged since it's blocking structures.

Argmaster and others added 3 commits March 20, 2025 17:02
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
}
source.worldEditData.clipboard = Blueprint.load(main.globalAllocator, storedBlueprint) catch |err| {
std.log.warn("Failed to load blueprint file '{s}' ({s})", .{fileName, @errorName(err)});
source.sendMessage("#ff0000Failed to load blueprint file '{s}' ({s})", .{fileName, @errorName(err)});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you do this everywhere now, how about a helper function to avoid duplicating the messages everywhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not resolved

Argmaster and others added 2 commits March 20, 2025 17:49
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
@Argmaster Argmaster force-pushed the feature/world-edit branch from d6c87ef to 11d8e4d Compare March 20, 2025 16:57
@Argmaster
Copy link
Collaborator Author

Btw extracting blueprint file io to struct made the code longer by about 20 lines 😝

@IntegratedQuantum
Copy link
Member

Btw extracting blueprint file io to struct made the code longer by about 20 lines 😝

Why did you do it then?

Copy link
Member

@IntegratedQuantum IntegratedQuantum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you did to the blueprint command really was that helpful, especially if you did increase the number of lines.

@Argmaster
Copy link
Collaborator Author

Argmaster commented Mar 20, 2025

Why did you do it then?

Cause I hoped it will be a good abstraction. I have reverted it tho, and moved just the opening into separate function, it is still +1 line of diff.

@IntegratedQuantum
Copy link
Member

How about just a helper function for the two output calls:

fn sendWarningAndLog(comptime fmt: []const u8, args: anytype) void {
    std.log.warn(fmt, args);
    user.sendMessage("#ff0000" ++ fmt, args);
}

@Argmaster
Copy link
Collaborator Author

Done.

@IntegratedQuantum
Copy link
Member

Did this finally save some lines instead of adding them?

Argmaster and others added 2 commits March 20, 2025 21:50
Co-authored-by: IntegratedQuantum <43880493+IntegratedQuantum@users.noreply.github.com>
@Argmaster
Copy link
Collaborator Author

Did this finally save some lines instead of adding them?

Yes, total of 4.

Copy link
Member

@IntegratedQuantum IntegratedQuantum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I think we are done here.

Honestly if it weren't needed for the structure models, I don't know if I would have had the motivation to keep reviewing this. In the future, please please please try to make smaller PRs. Reviewing 650 lines just takes so much time and energy, and I'm sure that's true on your side too.

@IntegratedQuantum IntegratedQuantum merged commit 37eb01e into PixelGuys:master Mar 20, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants