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

Fix regression where a tile's custom material was ignored #88190

Merged

Conversation

Eoin-ONeill-Yokai
Copy link
Contributor

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai commented Feb 11, 2024

Regression found via bisect; originates from commit 48bed50

Any material assigned to a tile via the TileSetEditor was ignored due to always using the parent material, even when a material is assigned to a tile.

I presume that the change I've made is close to the desired / expected behavior where a TileMap layer will only use a parent material if the material assigned is invalid. @groud Let me know if this is right.

Testing

  • Checkout master or commit 48bed50 and compile.
  • Try to assign a material to a tile via the TileSetEditor. The material will render properly in the editor directly.
  • Try painting the tile onto a TileMap. You'll notice that the material does not work as expected.
  • Check out this PR and recompile.
  • Materials should now work for individual tiles as well as TileMap/TileMapLayer(s).

Regression stemmed from commit 48bed50

The material assigned to a tile is completely ignored in master unless
this line is changed.
@Eoin-ONeill-Yokai Eoin-ONeill-Yokai requested a review from a team as a code owner February 11, 2024 02:15
@Eoin-ONeill-Yokai Eoin-ONeill-Yokai changed the title Fix regression where tileset's tile material was ignored Fix regression where a tiles custom material was ignored Feb 11, 2024
@clayjohn
Copy link
Member

Could probably still use a review from @groud But seems pretty safe to me

@AThousandShips AThousandShips changed the title Fix regression where a tiles custom material was ignored Fix regression where a tile's custom material was ignored Feb 11, 2024
Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

Yes, looks good!

@@ -328,7 +328,7 @@ void TileMapLayer::_rendering_update() {
rs->canvas_item_set_material(ci, mat->get_rid());
}
rs->canvas_item_set_parent(ci, get_canvas_item());
rs->canvas_item_set_use_parent_material(ci, true);
rs->canvas_item_set_use_parent_material(ci, !mat.is_valid());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rs->canvas_item_set_use_parent_material(ci, !mat.is_valid());
rs->canvas_item_set_use_parent_material(ci, mat.is_null());

@akien-mga akien-mga merged commit 8b72165 into godotengine:master Feb 13, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants