-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[render] RenderMaterial offers more nuanced and complete warnings #20236
[render] RenderMaterial offers more nuanced and complete warnings #20236
Conversation
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.
+@zachfang for feature review, please.
Reviewable status: LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
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.
Reviewed 11 of 11 files at r1.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/render_mesh.h
line 33 at r1 (raw file):
/* Indicates the degree that UV coordinates have been assigned to the mesh. only UvState::kFull supports texture images. No matter what, the `uvs` matrix
nit.
Suggestion:
Only
geometry/render/render_mesh.cc
line 273 at r1 (raw file):
// If we've only used a single material in the OBJ, use it. If the only // referenced material is "-1", it means there is no material. mesh_data.uv_state = has_tex_coord ? UvState::kFull : UvState::kNone;
BTW/question. To confirm, has_tex_coord
is a hit-or-miss from the current tinyobjloader
parsing. Thus kPartial
is not used other than the specific test cases we set up.
Code quote:
mesh_data.uv_state = has_tex_coord ? UvState::kFull : UvState::kNone;
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.
+@ggould-tri for platform review, please.
Reviewable status: LGTM missing from assignee ggould-tri(platform), missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/render_mesh.cc
line 273 at r1 (raw file):
Previously, zachfang wrote…
BTW/question. To confirm,
has_tex_coord
is a hit-or-miss from the currenttinyobjloader
parsing. ThuskPartial
is not used other than the specific test cases we set up.
The has_tex_coord
variable disappears in the next PR, replaced by meaningful computation of the uv state. At that point, we will be able to meaningfully indicate whether they have no UVs or just partial UVs.
The fallback materials would use the ('phong', 'diffuse_map') or matching foo.png images as diffuse texture map, even if the underlying mesh could not support it. This is because they were given no information about whether a texture could be applied or not. Now, it is provided information as to the support of texture coordinates and will *also* omit a texture if the underlying geometry lacks sufficient texture coordinate support. Note: This leads to a very subtle, but non-destructive behavior change. If a diffuse map was specified for a capsule in RenderEngineVtk, the image will not appear (as it did before), but now there's a warning emitted.
611a50b
to
ad75b5d
Compare
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.
Reviewed 2 of 2 files at r2.
Reviewable status: LGTM missing from assignee ggould-tri(platform), missing label for release notes (waiting on @SeanCurtis-TRI)
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.
as written but I have one design query below.
Reviewed 11 of 11 files at r1.
Reviewable status: 1 unresolved discussion, missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/render_material.h
line 15 at r1 (raw file):
/* Reports how UVs have been assigned to the mesh receiving a material. Textures should only be applied to meshes with *fully* assigned UVs. */ enum class UvState { kNone, kFull, kPartial };
minor: I don't understand how kPartial
comes about in our code. I see that kFull and kNone both occur in certain default conditions, but I don't see how we ever get kPartial.
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.
Reviewable status: missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/render_material.h
line 15 at r1 (raw file):
Previously, ggould-tri wrote…
minor: I don't understand how
kPartial
comes about in our code. I see that kFull and kNone both occur in certain default conditions, but I don't see how we ever get kPartial.
Zach had a similar question. It becomes meaningfully populated in the follow-up PR (the second commit in the linked epic PR).
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.
Reviewed 2 of 2 files at r2.
Reviewable status: missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/render_material.h
line 15 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Zach had a similar question. It becomes meaningfully populated in the follow-up PR (the second commit in the linked epic PR).
:) In the future perhaps put in a temporary TODO comment in such cases to spare yourself two needlessly confused reviewers.
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.
Reviewed all commit messages.
Reviewable status: missing label for release notes (waiting on @SeanCurtis-TRI)
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.
Reviewable status: missing label for release notes (waiting on @SeanCurtis-TRI)
geometry/render/render_material.h
line 15 at r1 (raw file):
Previously, ggould-tri wrote…
:) In the future perhaps put in a temporary TODO comment in such cases to spare yourself two needlessly confused reviewers.
Touche
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.
+(release notes: none)
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),zachfang
The fallback materials would use the ('phong', 'diffuse_map') or matching foo.png images as diffuse texture map, even if the underlying mesh could not support it. This is because they were given no information about whether a texture could be applied or not.
Now, it is provided information as to the support of texture coordinates and will also omit a texture if the underlying geometry lacks sufficient texture coordinate support.
Note: This leads to a very subtle, but non-destructive behavior change. If a diffuse map was specified for a capsule in RenderEngineVtk, the image will not appear (as it did before), but now there's a warning emitted.
This is the first PR in a train relating to #17266. The full PR train can be seen in #20235. This PR corresponds to the first commit in that PR.
This change is