Skip to content
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

Initial support for Terrain SmoothGrid data #444

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

checkraisefold
Copy link

@checkraisefold checkraisefold commented Aug 29, 2024

DISCLAIMER: THIS IS MY FIRST ACTUAL PROJECT IN RUST. Terrible code will bleach your eyes!
Because of that, this PR also has edits by maintainers on.

This PR adds support for serialization and deserialization to rbx-types for the Terrain object SmoothGrid property data.
This fully permits the creation of external tooling for modification and generation of smooth terrain data, as Roblox Studio, along with RCCService (presumably) generate the additionally-required PhysicsGrid data at runtime/publish-time.

As a bonus, no reverse engineering of the client had to be done here; just a lot of comparing outputs with ImHex peddled by this article.
https://zeux.io/2017/03/27/voxel-terrain-storage/

Rendered spec.

  • Decoder
  • Decoder tests
  • Encoder
  • Encoder tests
  • Module splitup

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

I'd prefer we keep the naming scheme intact and name the spec file for SmoothGrid to be smooth-grid.md.

Otherwise, I've left my comments and questions for the spec file.

docs/terrain.md Outdated Show resolved Hide resolved
docs/terrain.md Outdated Show resolved Hide resolved
docs/terrain.md Outdated Show resolved Hide resolved
docs/terrain.md Outdated Show resolved Hide resolved
docs/terrain.md Outdated Show resolved Hide resolved
docs/terrain.md Outdated Show resolved Hide resolved
docs/terrain.md Outdated Show resolved Hide resolved
docs/terrain.md Outdated Show resolved Hide resolved
docs/smooth-grid.md Outdated Show resolved Hide resolved
docs/smooth-grid.md Outdated Show resolved Hide resolved
docs/smooth-grid.md Outdated Show resolved Hide resolved
@checkraisefold
Copy link
Author

further code commits are paused until structure is reviewed so I don't add the deserializer and have to immediately rewrite it

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

Overall, I'd say that this is a good start to an encoder. The comments up above are more focused on the actual code than things like code structure.

Let's rename the terrain.rs module to smooth_grid.rs. I'd also like it if the TerrainSerializer trait could be killed off, since I don't think it adds anything. The encode and decode methods could be under the same impl blocks as the rest of the methods.

Architecturally I have some things I'd like to change but they involve files outside of this PR so I won't force them onto you.. Stuff like moving smooth_grid and material_colors into a submodule named terrain and having them share a Material enum, as an example. If you want to make those changes you're welcome to but otherwise I'll do it after this is merged.

docs/smooth-grid.md Outdated Show resolved Hide resolved
docs/smooth-grid.md Outdated Show resolved Hide resolved
rbx_types/src/variant.rs Outdated Show resolved Hide resolved
rbx_xml/src/types/mod.rs Outdated Show resolved Hide resolved
rbx_types/src/terrain.rs Outdated Show resolved Hide resolved
rbx_types/src/terrain.rs Outdated Show resolved Hide resolved
rbx_types/src/terrain.rs Outdated Show resolved Hide resolved
rbx_types/src/terrain.rs Outdated Show resolved Hide resolved
rbx_types/src/terrain.rs Outdated Show resolved Hide resolved
rbx_types/src/terrain.rs Outdated Show resolved Hide resolved
@checkraisefold
Copy link
Author

Architecturally I have some things I'd like to change but they involve files outside of this PR so I won't force them onto you.. Stuff like moving smooth_grid and material_colors into a submodule named terrain and having them share a Material enum, as an example. If you want to make those changes you're welcome to but otherwise I'll do it after this is merged.

i could give this a shot but I was mainly wondering how the enum should be structured with it being like this? the easiest way would be to just put it in order but you'd have to include air and some other non-colorable materials i think and then implement an error for them??

@checkraisefold
Copy link
Author

i could give this a shot but I was mainly wondering how the enum should be structured with it being like this? the easiest way would be to just put it in order but you'd have to include air and some other non-colorable materials i think and then implement an error for them??

for this

@Dekkonot
Copy link
Member

Dekkonot commented Sep 26, 2024

i could give this a shot but I was mainly wondering how the enum should be structured with it being like this? the easiest way would be to just put it in order but you'd have to include air and some other non-colorable materials i think and then implement an error for them??

There's a few ways I can imagine this working.

The first is that we just have a TerrainMaterial enum and error if you ever try to use a material that isn't allowed. This seems reasonable, but it's kind of a bummer that it would mean being unable to statically verify that a material was allowed.

Another option is to just say "to hell with it" and have multiple TerrainMaterial enums, which is how it's implemented right now. This solves the static analysis problem but it means that working with Terrain in rbx-dom requires two identically named enums, which is a terrible UX.

The final option I can see is that we have a centralized TerrainMaterial enum and then also module-specific material enums, which can be converted back and forth via from and try_from. I think I hate this idea, since it'd mean SmoothGridMaterial::try_from(TerrainMaterial::Water).unwrap() is a thing.

I'm inclined to go with the first option since it's the easiest and have the least obstructive UX possible. Moving us to a central TerrainMaterials enum is technically breaking because it involves adding variants to the one from MaterialColors, but I think I'm willing to accept it. Curious to know what @kennethloeffler thinks though.


In terms of actual implementation, I imagine that each Terrain would have its own internal variant that it used that it just tried to convert into from the central TerrainMaterials. That'd allow each module to do its own thing while still giving a consistent public interface.

@Dekkonot
Copy link
Member

Dekkonot commented Oct 7, 2024

Can't wait for Ken for the rest of our days, so I'm gonna go ahead and say go for the plan I gave above with moving us to a central TerrainMaterials enum.

Spec file, begin integrating it

whoops

fixes this ugly bit

reword

flat_map flat_map map

make nested for loops great again

The linter has spoken

requested changes

Spec file improvements, encoder improvements

Reword

Reword (2)

consistency

link water occupancy to shorelines

chunk max const

comments

current changes

docstring

naming convention
@checkraisefold
Copy link
Author

No that does not compile, just pushing work so far

@checkraisefold
Copy link
Author

So far my only hypothetical concern is performance - I feel like the algorithm in the encoder sucks
Decoder hopefully shouldn't be too bad in comparison at least

@checkraisefold
Copy link
Author

@Dekkonot
module splitup is done, re-requesting review of the splitup before I move on to the decoder

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.

2 participants