-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
base: master
Are you sure you want to change the base?
Conversation
…pendent of mouse position.
…dated versions later.
17k lines... O. M. G. That's for sure will take some time to review :) |
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. |
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. |
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. |
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. |
… rather than a bare [i8;9].
…olve issue with replacing list items.
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 I added |
…de when changing selection, and making tile set preview texture consistent.
fyrox-impl/src/scene/tilemap/edit.rs
Outdated
|
||
/// The state of a tile map that is being edited. | ||
#[derive(Default, Debug)] | ||
pub struct TileMapEditorData { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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>() { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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), | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
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.