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

Individual materials for GUI nodes #7606

Merged
merged 21 commits into from Jun 29, 2023

Conversation

Jhonnyg
Copy link
Contributor

@Jhonnyg Jhonnyg commented Apr 25, 2023

GUIs now has a list of materials that can be assigned to individual nodes, similar to textures, particles and fonts. If no material has been set on a node, the default GUI node material will be used.

Note: Assigning separate materials will break batching in the same manner as the regular GO world!

Fixes #7455

PR checklist

  • Code
    • Add engine and/or editor unit tests.
    • New and changed code follows the overall code style of existing code
    • Add comments where needed
  • Documentation
    • Make sure that API documentation is updated in code comments
    • Make sure that manuals are updated (in github.com/defold/doc)
  • Prepare pull request and affected issue for automatic release notes generator
    • Pull request - Write a message that explains what this pull request does. What was the problem? How was it solved? What are the changes to APIs or the new APIs introduced? This message will be used in the generated release notes. Make sure it is well written and understandable for a user of Defold.
    • Pull request - Write a pull request title that in a sentence summarises what the pull request does. Do not include "Issue-1234 ..." in the title. This text will be used in the generated release notes.
    • Pull request - Link the pull request to the issue(s) it is closing. Use on of the approved closing keywords.
    • Affected issue - Assign the issue to a project. Do not assign the pull request to a project if there is an issue which the pull request closes.
    • Affected issue - Assign the "breaking change" label to the issue if introducing a breaking change.
    • Affected issue - Assign the "skip release notes" is the issue should not be included in the generated release notes.

@Jhonnyg Jhonnyg marked this pull request as ready for review June 16, 2023 14:17
@Jhonnyg Jhonnyg requested review from matgis and JCash June 16, 2023 14:30
Copy link
Contributor

@matgis matgis left a comment

Choose a reason for hiding this comment

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

Big props for tackling the editor changes! The data dependencies in the Gui system are some of the most difficult to grasp in the entire editor code base. 😅

Before merging, we should add more validation for the selected material, and we could rename the new override-material-shader output if we included the default material in the material-names map under "" like we do for the textures.

editor/src/clj/editor/gui.clj Show resolved Hide resolved
editor/src/clj/editor/gui.clj Outdated Show resolved Hide resolved
editor/src/clj/editor/gui.clj Outdated Show resolved Hide resolved
editor/src/clj/editor/gui.clj Outdated Show resolved Hide resolved
@@ -2388,6 +2513,7 @@
(input layouts-node g/NodeID) ; for tests
(input fonts-node g/NodeID) ; for tests
(input textures-node g/NodeID) ; for tests
(input materials-node g/NodeID) ; for tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, did you write any tests? 😅

@Jhonnyg Jhonnyg added engine Issues related to the Defold engine editor Issues related to the Defold editor gui Issues related to gui components feature request A suggestion for a new feature labels Jun 26, 2023
@Jhonnyg Jhonnyg changed the title Individual materials for gui nodes work-in-progress Individual materials for GUI nodes Jun 26, 2023
matgis
matgis previously approved these changes Jun 27, 2023
Copy link
Contributor

@matgis matgis left a comment

Choose a reason for hiding this comment

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

Editor changes look good! Great work!

JCash
JCash previously approved these changes Jun 28, 2023
Copy link
Contributor

@JCash JCash left a comment

Choose a reason for hiding this comment

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

Lgtm.
Some minor detail regarding setting the materials at creation time.

Comment on lines 755 to 762
InternalNode* nodes = scene->m_Nodes.Begin();
for (uint32_t i = 0; i < scene->m_Nodes.Size(); ++i)
{
if (nodes[i].m_Node.m_MaterialNameHash == name_hash)
{
nodes[i].m_Node.m_Material = material;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think rather than looping for every node, for each material, we could have a loop outside of the function (at setup) where we lookup the materials from the map instead?

@Jhonnyg Jhonnyg dismissed stale reviews from JCash and matgis via bd72e42 June 29, 2023 09:59
@Jhonnyg Jhonnyg merged commit fce2e2a into dev Jun 29, 2023
20 checks passed
@Jhonnyg Jhonnyg deleted the issue-7455-individual-materials-on-gui-nodes branch June 29, 2023 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor Issues related to the Defold editor engine Issues related to the Defold engine feature request A suggestion for a new feature gui Issues related to gui components
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add option to set material on individual nodes
3 participants