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

[tri_homecart] Add KTX2 textures as extensionsUsed #37

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

jwnimmer-tri
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri commented Mar 5, 2024

This change is Reviewable

@jwnimmer-tri
Copy link
Contributor Author

+@SeanCurtis-TRI for both reviews, please.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

:LGTM: X2

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri] (waiting on @jwnimmer-tri)

a discussion (no related file):
The image below highlights the potential cost of ktx2 conversion.

The image on the left is the ktx2 texture image. Note the red splotches on the cutting board (absent from the original png on the right).

I mention it, not because it torpedos this PR but to highlight the care that needs to be taken and to document the need to refine the conversion process and revisit these textures. Or, failing that, come up with a sense of what kind of policy we want to adopt relative to these kinds of artifacts. We can assume that we're ok with meshcat having those artifacts because they won't show up in our render engines. I think there are multiple options available to us. Just as long as we actively choose them. Whatever we do, this might inform the documentation written in #21193.

I'll be playing with this later today anyways. It may well be that a tweak of process gives us loading benefits while preserving a greater amount of image fidelity.

cutting_board_ktx2.png


Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri] (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The image below highlights the potential cost of ktx2 conversion.

The image on the left is the ktx2 texture image. Note the red splotches on the cutting board (absent from the original png on the right).

I mention it, not because it torpedos this PR but to highlight the care that needs to be taken and to document the need to refine the conversion process and revisit these textures. Or, failing that, come up with a sense of what kind of policy we want to adopt relative to these kinds of artifacts. We can assume that we're ok with meshcat having those artifacts because they won't show up in our render engines. I think there are multiple options available to us. Just as long as we actively choose them. Whatever we do, this might inform the documentation written in #21193.

I'll be playing with this later today anyways. It may well be that a tweak of process gives us loading benefits while preserving a greater amount of image fidelity.

cutting_board_ktx2.png

From the PR number, I'm assuming that this was prepped a while ago. I ran the merged gltf_add_ktx2.py script this morning on the gltf and got a much better .ktx2 file. One without the red splotches (and one that happens to be ever so slightly smaller). Perhaps the new settings have already manifested an improvement?


Copy link
Contributor Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, platform LGTM from [seancurtis-tri] (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

From the PR number, I'm assuming that this was prepped a while ago. I ran the merged gltf_add_ktx2.py script this morning on the gltf and got a much better .ktx2 file. One without the red splotches (and one that happens to be ever so slightly smaller). Perhaps the new settings have already manifested an improvement?

Yeah, that was on my mind as well.

Would you be so kind as to push the current output of the conversion tool into this PR? Including any new gltf edits, if relevant.

We'll squash-merge onto master when finished.


Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, platform LGTM from [seancurtis-tri] (waiting on @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yeah, that was on my mind as well.

Would you be so kind as to push the current output of the conversion tool into this PR? Including any new gltf edits, if relevant.

We'll squash-merge onto master when finished.

I may have made a mistake...I'm observing some weird stuff.

We can go ahead and merge this as is and if I figure out what's going on, I can submit an update.


@jwnimmer-tri
Copy link
Contributor Author

Works for me.

@jwnimmer-tri jwnimmer-tri merged commit 0e790f7 into RobotLocomotion:master Mar 26, 2024
2 checks passed
@jwnimmer-tri jwnimmer-tri deleted the tri_homecart-ktx2 branch March 26, 2024 16:49
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.

2 participants