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

New System for Tile Map, Tile Set, and Brush #714

Open
wants to merge 46 commits into
base: master
Choose a base branch
from

Conversation

b-guild
Copy link
Contributor

@b-guild b-guild commented Dec 24, 2024

This tile map system is still a work in progress, but it is now a work in progress that is actually useable. It does everything the current tile map system can do and more. After weeks of working on it alone, I am making this pull request so that I can get some help with this enormous project. I cannot find all the problems with it myself, though I have tried and it seems to work quite well. There is just too much for one person.

It needs a lot more documentation and testing, but here are the major features that it already has:

  • Brushes and tile sets each contain multiple pages. Each page contains its own hashmap of tiles, and you can switch which page you are using by selecting a page from the area above the tiles, where each page is represented by a tile that serves as the icon for that page.
  • Brush pages allow a single brush to fill the role of multiple brushes. Instead of switching brushes, we can switch pages. Therefore I have simplified TileMap to remove the list of brushes that is no longer needed. If there is a need to switch brushes, the current brush can be changed by dragging a brush asset into the tile map control panel.
  • Tile set pages come in three flavors, each serving a different use:
    • The Material page allows you to specify a material and a tile size to easily create tiles from an atlas. Unlike the current system, this is not a one-time operation to create these tiles. Instead, the material can be replaced and the tile size can be changed at any time. Any tiles from that page in a tile map will adjust themselves accordingly, because tile map tiles know what page they are from and their position in the page.
    • The Freeform page allows you to create arbitrary tiles without needing an atlas. You can specify any material and the positions of the four corners of the tile in texture coordinates. You can put copies of existing tiles into the page by drawing them using the tile map control panel, much like drawing tiles to a tile map, but with the additional freedom to be able to flip and rotate any tile to any orientation. This allows the creation of mirrored or rotated versions of tiles from an atlas using the same material as the atlas but with different UVs.
    • The Transform Set page contains handles to tiles from other pages in groups of 8. These groups represent the ways in which a tile can be transformed by flipping and rotating. The 8 are arranged in a 2x4 rectangle. The left 2x2 area represents four 90-degree rotations, and the right 2x2 area represents those same rotations but horizontally mirrored. Thanks to this, we can flip and rotate tiles while editing a tile map, and the editor will automatically find the appropriate transformed version of the tile.
  • Collision now has multiple layers, with each layer identified by a name and UUID. The tile set editor has a tab for adding, removing, renaming, and changing the color of each layer. This is important because a single tile map will often need multiple physics bodies with different properties. Of course there is a solid rigid body for walls and floors, but often there will also be a need for icy surfaces and areas that the player can pass through, and so on. Each physics object can be assigned to a particular collision layer in the tile map.
  • A custom collider option has been added, allowing the user to specify arbitrary triangles. There is currently no GUI interface for this system; the user needs to type the triangles in as a list of points and index-triples in a provided text area. This is a clear place where improvement is possible.
  • Properties are now shared by all tiles in a tile set. Each tile does not have its own independent list of properties, but rather each tile set has a list of properties, and each tile has only its own values for those properties. This creates consistency across the tile set, with each property having a particular type, and optional list of named pre-defined values, and an order in which the properties will be presented in the tile set editor.
  • Tile map tool randomness is now a option that can be toggled using a button with a dice icon. A randomized rect fill will use random tiles from the current selection, while a non-randomized rect fill will repeat the current selection as many times as necessary to fill the area.
  • A preview widget shows the current selected tiles and adjusts itself to show those tiles after the rotations and flips have been applied. The tile map control panel now has rotation and flip buttons, though they are only fully effective for tiles that contain rotations and flips in some Transform Set page of the tile set.

@mrDIMAS
Copy link
Member

mrDIMAS commented Dec 24, 2024

17k lines... O. M. G. That's for sure will take some time to review :)

@b-guild
Copy link
Contributor Author

b-guild commented Dec 24, 2024

I think it might be wise to rename material pages to atlas pages and change the names of related concepts to match. I may not have given enough thought to what things are named in this project, so there may be other names that should be changed.

@b-guild
Copy link
Contributor Author

b-guild commented Dec 26, 2024

If anything is in any way confusing or unclear, I would be very happy to add comments, rename things, re-organize things, or in other ways adjust the PR so that it is easier to understand.

@mrDIMAS
Copy link
Member

mrDIMAS commented Dec 26, 2024

I'm on the review, the PR is just... too large. I mean it is probably one of the largest PR I've ever seen including ones at work. I'm half way through it.

@b-guild
Copy link
Contributor Author

b-guild commented Dec 27, 2024

It would be great to have advice on method names, parameter order, and generally making sure that everything fits in with a consistent Fyrox style.

@b-guild
Copy link
Contributor Author

b-guild commented Dec 28, 2024

I discovered a difficult bug with some of the dropdown lists in the tile set editor and I tracked it down to an issue with the way that ListViewMessage::Items is processed. I was not able to find a work-around, but I was able to resolve the issue by modifying ListView and adding a new WidgetMessage variant. This is an extreme and experimental solution, but it seems to work well, and I don't know how to solve the problem otherwise.

I added WidgetMessage::ReplaceChildren which removes whatever children the node has and replaces them with a given list of children. This is different from what ListViewMessage::Items currently does because ListViewMessage::Items depends upon knowing the children that will be removed before it sends the messages which assumes that no more children will be added before the messages are processed, but this assumption is not necessarily true. WidgetMessage::ReplaceChildren waits to determine the removed nodes at the time the message is processed rather than at the time the message is sent, which makes it less likely to produce unpleasant surprises. You always get exactly the children that you ask for.

editor/src/plugins/tilemap/tile_set_import.rs Outdated Show resolved Hide resolved
fyrox-impl/src/scene/tilemap/edit.rs Outdated Show resolved Hide resolved
editor/src/plugins/tilemap/tile_bounds_editor.rs Outdated Show resolved Hide resolved
editor/src/plugins/tilemap/tileset.rs Outdated Show resolved Hide resolved
editor/src/plugins/tilemap/tileset.rs Outdated Show resolved Hide resolved
fyrox-impl/src/scene/tilemap/mod.rs Outdated Show resolved Hide resolved
fyrox-impl/src/scene/tilemap/mod.rs Outdated Show resolved Hide resolved
fyrox-impl/src/scene/tilemap/mod.rs Outdated Show resolved Hide resolved

/// The state of a tile map that is being edited.
#[derive(Default, Debug)]
pub struct TileMapEditorData {
Copy link
Member

Choose a reason for hiding this comment

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

Editor-specific data shouldn't be in the engine code. This violates strict separation between the editor and the engine that was in the project from the very beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not refer to anything specific to the Fyrox editor. It is a general-purpose facility that any editor might find useful, even perhaps an in-game level editor. It just changes how the tile map renders itself in ways that happen to be useful to the Fyrox editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certain important effects would be far more awkward to achieve if we could not control how the tile map is rendered and we are forced to work around a tile map that always renderers itself normally. It is especially important to be able to tell a tile map to not render tiles at certain positions without needing to actually remove those tiles. If we must remove all specialized rendering from the TileMap node, then I would suggest that we should create an editor-only version of the node that is added when the interaction mode is activated while the actual TileMap node is made invisible.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please rename it to something else that won't contain editor? Previous version contained overlay_tiles field which was used in the editor only, but didn't specifically tell that in the engine code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this name has inspired me to also change how it works. I am working on a general-purpose system that games can use to change how the tile map renderers without changing the underlying data: making tiles invisible, temporarily replacing one tile with another, arbitrary rendering, and so on. It will do everything that the editor needs, plus games should be able to create effects beyond what the editor needs.

This also reminds me that animated tiles should be possible. I am unsure how that should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new TileMapEffect is a significantly superior design that simplifies the code, makes it easier to understand, and is far more flexible and expandable. While TileMapEditorData was a monolithic do-everything object with no clear purpose, each TileMapEffect does one particular thing, and TileMap can have any number of different TileMapEffect objects in a list, and each one can change how TileMap renders in potentially versatile ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discovered that sometimes a TileMapEffect needs to happen after the tile map is rendered, so I had to split TileMap::effects into two lists, TileMap::before_effects and TileMap::after_effects. Now it seems to be working as intended.

fyrox-impl/src/scene/tilemap/tileset.rs Outdated Show resolved Hide resolved
fn handle_routed_message(&mut self, ui: &mut UserInterface, message: &mut UiMessage) {
self.widget.handle_routed_message(ui, message);

if let Some(PaletteMessage::SyncToState) = message.data::<PaletteMessage>() {
Copy link
Member

Choose a reason for hiding this comment

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

This does not check for destination node nor for direction, is this indented?

}

fn make_highlight_vertex(transform: &Matrix4<f32>, position: Vector2<f32>) -> StaticVertex {
StaticVertex {
Copy link
Member

@mrDIMAS mrDIMAS Jan 4, 2025

Choose a reason for hiding this comment

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

Ideally, tile map should use specialized vertex format to reduce memory usage. This vertex format is specific for 3d, because it contains normal and tangent (which are non-existent in 2d). This can be done later in other PR.

]
.map(TriangleDefinition);

let sort_index = 0; //ctx.calculate_sorting_index(render_position.position());
Copy link
Member

Choose a reason for hiding this comment

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

This one is confusing. I suppose highlighting should be always on top, and thus this index should have some large value, and definitely not zero. I maybe missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an experiment from a time when the highlight rendering was not working properly, and after I got the highlight working properly I forgot to undo the experiment since there was no need to fix something that was not broken. I do not actually know what sort_index does. It would be nice to have detailed documentation for RenderDataBundleStorageTrait because rendering can be quite a complex topic and some people need all the help they can get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an experiment from a time when the highlight rendering was not working properly, and after I got the highlight working properly I forgot to undo the experiment since there was no need to fix something that was not broken. I do not actually know what sort_index does. It would be nice to have detailed documentation for RenderDataBundleStorageTrait because rendering can be quite a complex topic and some people need all the help they can get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that a sort_index of 0 works, but using calculate_sorting_index causes the cursor to prevent the selection from rendering. I tried adding 1 to the sorting index of the selection, but it turns out that calculate_sorting_index was returning u64::MAX, so adding one caused a crash. Next I tried saturating_sub(1), which I suppose causes the selection to render before the cursor, thereby allowing them both to appear.

I am far from a shader expert, but it seems that the shader is checking the depth buffer before rendering, so things that render earlier at minimum depth can prevent things from rendering later at minimum depth. That depth check probably should not exist when we create a 2D version of this shader.

Comment on lines +748 to +752
match self {
TileBook::Empty => None,
TileBook::TileSet(resource) => resource.state().data()?.get_tile_render_data(position),
TileBook::Brush(resource) => resource.state().data()?.get_tile_render_data(position),
}
Copy link
Member

Choose a reason for hiding this comment

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

This will most likely be a performance bottleneck in case of large tile maps, especially if the tiles are small and lots of them are on screen at the same time. It will be the bottleneck because resource.state() locks a mutex internally and doing so thousands of times per frame will most likely eat some CPU time. I suggest to split rendering code in collect_render_data to lock the mutex only once (based on the type of the tile book) and basically make a method that will accept the resource data and render tiles with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TileBook::get_tile_render_data is never called in collect_render_data because the TileBook abstraction is never used by the TileMap node. TileBook is a way to treat both TileSet and TileMapBrush as interchangeable, but TileMap only ever uses TileSet. TileBook is used in the Tile Set Editor to make it possible to also edit brushes, but you cannot give a brush to a tile map where a tile set is expected. I will add a comment making note of the slowness so future people will not abuse this method.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, it is hard to grasp what's really going on in such large code base.

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