-
-
Notifications
You must be signed in to change notification settings - Fork 364
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
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. |
] | ||
.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.
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.
Finally, I finished the review. There are very few small issues in general, could you please fix them and I'll merge the PR right after.
editor/src/plugins/tilemap/mod.rs
Outdated
Erase, | ||
/// Flood a the cells of a tile map, replacing regions of identical cell. This operation is only possible on a tile map. | ||
/// For tile set and brush editing, this oepration behaves exactly like [`DrawingMode::Draw`]. |
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.
Type oepration
.
editor/src/plugins/tilemap/mod.rs
Outdated
scene | ||
.graph | ||
.try_get(*h) | ||
.and_then(|n| n.component_ref::<TileMap>()) |
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.
JFYI you could use scene.graph.try_get_of_type::<TileMap>(*h)
instead, it does exactly the same as try_get
+ component_ref
.
if self.kind != TilePaletteStage::Tiles { | ||
panic!(); | ||
} |
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 might be better to use assert_eq!(self.kind, TilePaletteStage::Tiles)
} | ||
} | ||
fn accept_material_drop(&mut self, _material: MaterialResource, _ui: &UserInterface) { | ||
// TODO |
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.
What's this todo about? Could you please add more info so it can be done later without seeking for additional info?
As I was trying to take some screen shots for my future book PR, I managed to make the editor crash. I will have to fix that problem before this PR is merged. |
Should I merge the PR or you have something to change? The stuff that I highlighted is not critical and I can merge without fixing those tiny issues. |
I think there are no more problems to fix. If I find any problems I can fix them in the animation PR that I'll do next. That PR is almost ready to go. I'll just do the book PR first. |
1 similar comment
I think there are no more problems to fix. If I find any problems I can fix them in the animation PR that I'll do next. That PR is almost ready to go. I'll just do the book PR first. |
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.