Fix TileMapLayer tiles displaying incorrectly with y_sort based on visibility layer#110550
Fix TileMapLayer tiles displaying incorrectly with y_sort based on visibility layer#110550Repiteo merged 1 commit intogodotengine:masterfrom
TileMapLayer tiles displaying incorrectly with y_sort based on visibility layer#110550Conversation
WesleyClements
changed the title
TileMapLayer tiles displaying incorrectly with y_sort based on visibility layer
AThousandShips
changed the title
TileMapLayer tiles displaying incorrectly with y_sort based on visibility layerTileMapLayer tiles displaying incorrectly with y_sort based on visibility layer
|
ysort_test.zip |
AThousandShips
added
cherrypick:4.4
Such offsets are valid only while counting/gathering the items to be y-sorted. After all the gathered items are actually y-sorted these previously calculated offsets are no longer relevant. Currently in this PR the skipping is applied after the sorting, aka the solution is incorrect. This PR in action: godot.windows.editor.x86_64_YNB8FU6pLi.mp4I think what should be sufficient is extending these to include visibility layer check: and marking y-sorting as dirty on changing given item's visibility layer, just like it's done on changing the visibility: godot/servers/rendering/renderer_canvas_cull.cpp Lines 645 to 650 in adb2ec0 godot/servers/rendering/renderer_canvas_cull.cpp Lines 613 to 620 in adb2ec0 This should make the relevant items (and their children/subtrees) be not even included in |
|
@kleonc Great minds think alike! This was the first solution I tried! The problem is that if the tree is used by multiple viewports (as in they share World2Ds) with different cull masks, then the cached ysort_children_count will only be valid for one of them which causes the others to crash the game. This solution was a misguided keep the caching behavior but it seems it is incomplete. It would be simple enough just filter by visibility_layer and do away with the caching. I don't know which is best but I'll implement the suggestion. Regardless, marking ysort as dirty in canvas_item_set_visibility_layer should happen. Edit: |
WesleyClements
force-pushed
the
master
branch
from
2ecaba5 to
259654b
Compare
Oh, I haven't considered that indeed. I see the issue, you're absolutely right about it potentially crashing, thus I think such solution is not acceptable. A solution I can think of which should work fine with multiple viewports sharing World2D would be to add another cached value for y-sorting to the item, specifically cumulated visibility layer (bitwise and-ed with the y-sorted item's parents (only the parents within the y-sorted hierarchy need to be accounted though)). Something like: diff --git a/servers/rendering/renderer_canvas_cull.cpp b/servers/rendering/renderer_canvas_cull.cpp
index 3b91bace7e..f0b635363d 100644
--- a/servers/rendering/renderer_canvas_cull.cpp
+++ b/servers/rendering/renderer_canvas_cull.cpp
@@ -132,6 +132,7 @@ void RendererCanvasCull::_collect_ysort_children(RendererCanvasCull::Item *p_can
child_items[i]->ysort_modulate = p_modulate;
child_items[i]->ysort_index = r_index;
child_items[i]->ysort_parent_abs_z_index = p_z;
+ child_items[i]->ysort_visibility_layer = p_canvas_item->ysort_visibility_layer & child_items[i]->visibility_layer;
if (!child_items[i]->repeat_source) {
child_items[i]->repeat_size = p_canvas_item->repeat_size;
@@ -437,6 +438,7 @@ void RendererCanvasCull::_cull_canvas_item(Item *p_canvas_item, const Transform2
ci->ysort_modulate = Color(1, 1, 1, 1) / ci->modulate;
ci->ysort_index = 0;
ci->ysort_parent_abs_z_index = parent_z;
+ ci->ysort_visibility_layer = ci->visibility_layer;
child_items[0] = ci;
int i = 1;
_collect_ysort_children(ci, p_material_owner, Color(1, 1, 1, 1), child_items, i, p_z);
@@ -445,7 +447,9 @@ void RendererCanvasCull::_cull_canvas_item(Item *p_canvas_item, const Transform2
sorter.sort(child_items, child_item_count);
for (i = 0; i < child_item_count; i++) {
- _cull_canvas_item(child_items[i], final_xform * child_items[i]->ysort_xform, p_clip_rect, modulate * child_items[i]->ysort_modulate, child_items[i]->ysort_parent_abs_z_index, r_z_list, r_z_last_list, (Item *)ci->final_clip_owner, (Item *)child_items[i]->material_owner, true, p_canvas_cull_mask, child_items[i]->repeat_size, child_items[i]->repeat_times, child_items[i]->repeat_source_item);
+ if (child_items[i]->ysort_visibility_layer & p_canvas_cull_mask) {
+ _cull_canvas_item(child_items[i], final_xform * child_items[i]->ysort_xform, p_clip_rect, modulate * child_items[i]->ysort_modulate, child_items[i]->ysort_parent_abs_z_index, r_z_list, r_z_last_list, (Item *)ci->final_clip_owner, (Item *)child_items[i]->material_owner, true, p_canvas_cull_mask, child_items[i]->repeat_size, child_items[i]->repeat_times, child_items[i]->repeat_source_item);
+ }
}
} else {
RendererCanvasRender::Item *canvas_group_from = nullptr;
diff --git a/servers/rendering/renderer_canvas_cull.h b/servers/rendering/renderer_canvas_cull.h
index 1f01bbce11..072e06b680 100644
--- a/servers/rendering/renderer_canvas_cull.h
+++ b/servers/rendering/renderer_canvas_cull.h
@@ -57,6 +57,7 @@ public:
Transform2D ysort_xform; // Relative to y-sorted subtree's root item (identity for such root). Its `origin.y` is used for sorting.
int ysort_index;
int ysort_parent_abs_z_index; // Absolute Z index of parent. Only populated and used when y-sorting.
+ uint32_t ysort_visibility_layer;
uint32_t visibility_layer = 0xffffffff;
Vector<Item *> child_items;
Note that |
WesleyClements
force-pushed
the
master
branch
from
259654b to
831e06f
Compare
|
@kleonc I have implemented it I believe. struct YSortItem {
Item* item;
uint32_t cumulative_visibility_layer;
}
Edit: |
WesleyClements
force-pushed
the
master
branch
from
831e06f to
f78dfbb
Compare
|
@kleonc I've thought about this a bit more and figured a way to solve the bug that doesn't add more properties to Item, doesn't add more allocation overhead, and prevents sorting items that won't be rendered. diff --git a/servers/rendering/renderer_canvas_cull.cpp b/servers/rendering/renderer_canvas_cull.cpp
index 3b91bace7e..42ba93c0f2 100644
--- a/servers/rendering/renderer_canvas_cull.cpp
+++ b/servers/rendering/renderer_canvas_cull.cpp
@@ -107,50 +107,57 @@ void RendererCanvasCull::_render_canvas_item_tree(RID p_to_render_target, Canvas
}
}
-void RendererCanvasCull::_collect_ysort_children(RendererCanvasCull::Item *p_canvas_item, RendererCanvasCull::Item *p_material_owner, const Color &p_modulate, RendererCanvasCull::Item **r_items, int &r_index, int p_z) {
+void RendererCanvasCull::_collect_ysort_children(RendererCanvasCull::Item *p_canvas_item, RendererCanvasCull::Item *p_material_owner, const Color &p_modulate, RendererCanvasCull::Item **r_items, int &r_index, int &r_ysort_children_count, int p_z, uint32_t p_canvas_cull_mask) {
int child_item_count = p_canvas_item->child_items.size();
RendererCanvasCull::Item **child_items = p_canvas_item->child_items.ptrw();
for (int i = 0; i < child_item_count; i++) {
if (child_items[i]->visible) {
- // To y-sort according to the item's final position, physics interpolation
- // and transform snapping need to be applied before y-sorting.
- Transform2D child_xform;
- if (!_interpolation_data.interpolation_enabled || !child_items[i]->interpolated) {
- child_xform = child_items[i]->xform_curr;
- } else {
- real_t f = Engine::get_singleton()->get_physics_interpolation_fraction();
- TransformInterpolator::interpolate_transform_2d(child_items[i]->xform_prev, child_items[i]->xform_curr, child_xform, f);
- }
+ if (child_items[i]->visibility_layer & p_canvas_cull_mask) {
+ // To y-sort according to the item's final position, physics interpolation
+ // and transform snapping need to be applied before y-sorting.
+ Transform2D child_xform;
+ if (!_interpolation_data.interpolation_enabled || !child_items[i]->interpolated) {
+ child_xform = child_items[i]->xform_curr;
+ } else {
+ real_t f = Engine::get_singleton()->get_physics_interpolation_fraction();
+ TransformInterpolator::interpolate_transform_2d(child_items[i]->xform_prev, child_items[i]->xform_curr, child_xform, f);
+ }
- if (snapping_2d_transforms_to_pixel) {
- child_xform.columns[2] = (child_xform.columns[2] + Point2(0.5, 0.5)).floor();
- }
+ if (snapping_2d_transforms_to_pixel) {
+ child_xform.columns[2] = (child_xform.columns[2] + Point2(0.5, 0.5)).floor();
+ }
- r_items[r_index] = child_items[i];
- child_items[i]->ysort_xform = p_canvas_item->ysort_xform * child_xform;
- child_items[i]->material_owner = child_items[i]->use_parent_material ? p_material_owner : nullptr;
- child_items[i]->ysort_modulate = p_modulate;
- child_items[i]->ysort_index = r_index;
- child_items[i]->ysort_parent_abs_z_index = p_z;
-
- if (!child_items[i]->repeat_source) {
- child_items[i]->repeat_size = p_canvas_item->repeat_size;
- child_items[i]->repeat_times = p_canvas_item->repeat_times;
- child_items[i]->repeat_source_item = p_canvas_item->repeat_source_item;
- }
+ r_items[r_index] = child_items[i];
+ child_items[i]->ysort_xform = p_canvas_item->ysort_xform * child_xform;
+ child_items[i]->material_owner = child_items[i]->use_parent_material ? p_material_owner : nullptr;
+ child_items[i]->ysort_modulate = p_modulate;
+ child_items[i]->ysort_index = r_index;
+ child_items[i]->ysort_parent_abs_z_index = p_z;
+
+ if (!child_items[i]->repeat_source) {
+ child_items[i]->repeat_size = p_canvas_item->repeat_size;
+ child_items[i]->repeat_times = p_canvas_item->repeat_times;
+ child_items[i]->repeat_source_item = p_canvas_item->repeat_source_item;
+ }
- // Y sorted canvas items are flattened into r_items. Calculate their absolute z index to use when rendering r_items.
- int abs_z = 0;
- if (child_items[i]->z_relative) {
- abs_z = CLAMP(p_z + child_items[i]->z_index, RS::CANVAS_ITEM_Z_MIN, RS::CANVAS_ITEM_Z_MAX);
- } else {
- abs_z = child_items[i]->z_index;
- }
+ // Y sorted canvas items are flattened into r_items. Calculate their absolute z index to use when rendering r_items.
+ int abs_z = 0;
+ if (child_items[i]->z_relative) {
+ abs_z = CLAMP(p_z + child_items[i]->z_index, RS::CANVAS_ITEM_Z_MIN, RS::CANVAS_ITEM_Z_MAX);
+ } else {
+ abs_z = child_items[i]->z_index;
+ }
- r_index++;
+ r_index++;
- if (child_items[i]->sort_y) {
- _collect_ysort_children(child_items[i], child_items[i]->use_parent_material ? p_material_owner : child_items[i], p_modulate * child_items[i]->modulate, r_items, r_index, abs_z);
+ if (child_items[i]->sort_y) {
+ _collect_ysort_children(child_items[i], child_items[i]->use_parent_material ? p_material_owner : child_items[i], p_modulate * child_items[i]->modulate, r_items, r_index,
r_ysort_children_count, abs_z, p_canvas_cull_mask);
+ }
+ } else {
+ r_ysort_children_count--;
+ if (child_items[i]->sort_y) {
+ r_ysort_children_count -= child_items[i]->ysort_children_count;
+ }
}
}
}
@@ -164,7 +171,10 @@ int RendererCanvasCull::_count_ysort_children(RendererCanvasCull::Item *p_canvas
if (child_items[i]->visible) {
ysort_children_count++;
if (child_items[i]->sort_y) {
- ysort_children_count += _count_ysort_children(child_items[i]);
+ if (child_items[i]->ysort_children_count == -1) {
+ child_items[i]->ysort_children_count = _count_ysort_children(child_items[i]);
+ }
+ ysort_children_count += child_items[i]->ysort_children_count;
}
}
}
@@ -439,7 +449,7 @@ void RendererCanvasCull::_cull_canvas_item(Item *p_canvas_item, const Transform2
ci->ysort_parent_abs_z_index = parent_z;
child_items[0] = ci;
int i = 1;
- _collect_ysort_children(ci, p_material_owner, Color(1, 1, 1, 1), child_items, i, p_z);
+ _collect_ysort_children(ci, p_material_owner, Color(1, 1, 1, 1), child_items, i, child_item_count, p_z, p_canvas_cull_mask);
SortArray<Item *, ItemYSort> sorter;
sorter.sort(child_items, child_item_count);
diff --git a/servers/rendering/renderer_canvas_cull.h b/servers/rendering/renderer_canvas_cull.h
index 1f01bbce11..c83fa1fb66 100644
--- a/servers/rendering/renderer_canvas_cull.h
+++ b/servers/rendering/renderer_canvas_cull.h
@@ -207,7 +207,7 @@ private:
void _render_canvas_item_tree(RID p_to_render_target, Canvas::ChildItem *p_child_items, int p_child_item_count, const Transform2D &p_transform, const Rect2 &p_clip_rect, const Color &p_modulate, Render
erCanvasRender::Light *p_lights, RendererCanvasRender::Light *p_directional_lights, RS::CanvasItemTextureFilter p_default_filter, RS::CanvasItemTextureRepeat p_default_repeat, bool p_snap_2d_vertices_to_pixel,
uint32_t p_canvas_cull_mask, RenderingMethod::RenderInfo *r_render_info = nullptr);
void _cull_canvas_item(Item *p_canvas_item, const Transform2D &p_parent_xform, const Rect2 &p_clip_rect, const Color &p_modulate, int p_z, RendererCanvasRender::Item **r_z_list, RendererCanvasRender::I
tem **r_z_last_list, Item *p_canvas_clip, Item *p_material_owner, bool p_is_already_y_sorted, uint32_t p_canvas_cull_mask, const Point2 &p_repeat_size, int p_repeat_times, RendererCanvasRender::Item *p_repeat_
source_item);
- void _collect_ysort_children(RendererCanvasCull::Item *p_canvas_item, RendererCanvasCull::Item *p_material_owner, const Color &p_modulate, RendererCanvasCull::Item **r_items, int &r_index, int p_z);
+ void _collect_ysort_children(RendererCanvasCull::Item *p_canvas_item, RendererCanvasCull::Item *p_material_owner, const Color &p_modulate, RendererCanvasCull::Item **r_items, int &r_index, int &r_ysort
_children_count, int p_z, uint32_t p_canvas_cull_mask);
int _count_ysort_children(RendererCanvasCull::Item *p_canvas_item);
void _mark_ysort_dirty(RendererCanvasCull::Item *ysort_owner);Effectively when collecting the children, if an Item doesn't pass the |
WesleyClements
force-pushed
the
master
branch
from
f78dfbb to
89503e3
Compare
|
@WesleyClements Actually after rethinking my previous suggestion was incorrect in the first place, specifically combining the layers and-wise. E.g. when we have y-sorted: Then such child would never be rendered, as and-wise combined layer for the child would be 0. But it should be rendered if the cull mask includes both layer 1 and layer 2 (that's how it works without y-sorting).
... and it doesn't have the layer checking issue I've just mentioned above. :)
All makes perfect sense to me, good job! The optimization in godot/servers/rendering/renderer_canvas_cull.cpp Lines 174 to 179 in 149a4b4 |
kleonc
left a comment
kleonc
left a comment
There was a problem hiding this comment.
LGTM! Fixes the issue, and should work fine for rendering the same canvas/World2D for Viewports with different cull masks.
@WesleyClements BTW if possible, you could update the PR description (first comment) as it's quite outdated currently.
|
Thanks! Congratulations on your first merged contribution! 🎉 |
akien-mga
changed the title
TileMapLayer tiles displaying incorrectly with y_sort based on visibility layerTileMapLayer tiles displaying incorrectly with y_sort based on visibility layer
|
Cherry-picked for 4.5.2. |
akien-mga
removed
cherrypick:4.4
5 participants
Resolves #75206
These changes address an oversight of the y sorting logic that made it possible for children of CanvasItems that should be culled by
canvas_cull_maskto still be rendered.For example, if:
y_sort_enabledset to true andvisibility_layerset to 1y_sort_enabledset to true, andvisibility_layerset to 2visibility_layerset to 1CanvasItem C would still render in any and all viewports with layer 1 enabled.
This was due to the fact that the
child_itemsarray included all children, filtered only byvisible. Since each element inchild_itemsis then checked to be culled individually, the parent'svisibility_layeris completely disregarded.To resolve this, I added two new parameters to
RendererCanvasCull::_collect_ysort_children,r_ysort_children_countandp_canvas_cull_mask. If an item matches (while visible) thep_canvas_cull_mask, it is added tochild_itemsas before. If it doesn't match (while visible), it skips adding itself to the array and subtracts itself and it'sysort_children_count(if it has any) from the length of the array,r_ysort_children_count. To avoid having to callRendererCanvasCull::_count_ysort_childrenagain to determine the size of the branch being subtracted from the array length, I changedRendererCanvasCull::_count_ysort_childrento cache the computedysort_children_countall the way down the tree.While this solution does not resolve the fact that
child_itemsis being allocated to be too large, it works as intended.This fixes the bug with TileMaps/TileMapLayers because each tile is added as a child CanvasItem with
visibility_layerset to the default of 0xffffffff. Thus, if the TileMapLayer and it's parent both havey_sort_enabledset to true but differ in passing thep_canvas_cull_maskcheck, the TileMapLayers children will still be rendered on viewports matching the parent'svisibility_layer.