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

Texture groups #165

Merged
merged 22 commits into from
Aug 18, 2023
Merged

Texture groups #165

merged 22 commits into from
Aug 18, 2023

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Aug 17, 2023

This PR introduces a concept of texture groups. Elements like floors and walls will no longer have their own texture properties but instead will be affiliated with a TextureGroup that describes the texture properties. Changing the properties of one texture group will affect all elements affiliated with the group.

Individual elements can be moved between texture groups at any time or become "unaffiliated" in which case they will be rendered with a simple white texture (in a future PR we can consider having a special "unaffiliated" texture to make these cases more obvious).

Texture groups can be renamed, merged, or deleted at any time by expanding the Group menu in the right panel.

mxgrey and others added 4 commits August 17, 2023 13:48
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Co-authored-by: Michael X. Grey <[email protected]>
Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Looks great! I tried it in a variety of cases and the experience is very pleasant and smooth. Just a few small points:

Texture groups can be renamed, merged, or deleted at any time by expanding the Group menu in the right panel.

I didn't see this in the UI and when looking at the code I couldn't find any obvious "Group" menu, am I missing it or is it not part of the PR?

Another minor issue I found is when importing legacy projects with materials that have same name but different properties. For example when importing this project, there are several white_wall textures that only have a different alpha value, they are all imported with the same name white_wall.
While the UI explicitly forbids creating groups with the same name this will be done automatically on project import, I wonder if, for example, adding a progressive number to duplicates could be an idea? I'm not sure if other parts of the code could behave unexpectedly if the unique ID assumption is violated.

rmf_site_editor/src/widgets/menu_bar.rs Outdated Show resolved Hide resolved
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey
Copy link
Collaborator Author

mxgrey commented Aug 17, 2023

I didn't see this in the UI and when looking at the code I couldn't find any obvious "Group" menu

Thanks for pointing this out, I had been working across multiple devices and forgot to push changes from one of them. You should be able to see it now. Here's a screenshot to give a hint:

groups

I'm not sure if other parts of the code could behave unexpectedly if the unique ID assumption is violated.

Forbidding different texture groups from using the same name is really just for the sake of UX. Drop down menus don't have as nice of a user experience when multiple options display the same text. I want the implementation of site editor to always be robust to duplicate names, so we should never have to worry about wrong behavior in the presence of duplicate names; only worse user experience.

This is also something that will be flagged by the upcoming diagnostic tool since the group name are stored in the NameInSite component.

adding a progressive number to duplicates could be an idea

That's certainly an option. I'm okay with or without it. We could save that for a follow up PR after we've spent some time with the features as-is.

#[cfg_attr(feature = "bevy", derive(Bundle))]
pub struct TextureGroup {
pub name: NameInSite,
#[serde(flatten)]
Copy link
Member

Choose a reason for hiding this comment

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

I believe this would break file saving / loading if merged (as documented in #166)

arjo129 and others added 5 commits August 18, 2023 15:47
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
@mxgrey mxgrey merged commit 33f1e3e into luca/floor_wall_materials Aug 18, 2023
5 checks passed
@mxgrey mxgrey deleted the texture_groups branch August 18, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants