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

Add automated regression test #4

Closed
Tracked by #13
jwnimmer-tri opened this issue Mar 21, 2023 · 10 comments · Fixed by #28
Closed
Tracked by #13

Add automated regression test #4

jwnimmer-tri opened this issue Mar 21, 2023 · 10 comments · Fixed by #28
Assignees

Comments

@jwnimmer-tri
Copy link
Contributor

jwnimmer-tri commented Mar 21, 2023

With a sample *.gltf + *.blend file, use https://docs.python.org/3.10/library/urllib.request.html to call the RPC and check the image that comes back.

Note that in particular, no Drake code is used in this automated regression test.

@jwnimmer-tri
Copy link
Contributor Author

We can add the new test case(s) to the existing server_test.py.

@jwnimmer-tri
Copy link
Contributor Author

jwnimmer-tri commented Apr 27, 2023

(See also #14, for the ability to load *.blend in the first place, even without an image-check test.)

@jwnimmer-tri
Copy link
Contributor Author

For this test it's OK to use a different *.blend file for this test than the one I tried in #12. It sounds like using a simpler *.blend file to start would work better.

Note however that we cannot use a blend file with the TRI logo. Any test data we add to this repository will be open-sourced, and we shouldn't open-source our logo.

@zachfang zachfang self-assigned this Apr 27, 2023
@zachfang
Copy link
Contributor

The models included in the blend file will not appear in the input glTF file, though. Thus, there may be two types of tests:

(1) Just test the glTF-in-image-out (or load a blend file without any geometry but lighting) with a golden image and a matching glTF file.
(2) The golden image has all the defined geometries rendered, and the test renders geometries come from either the blend file or the glTF file.

The first one is very clear to me, but I am not sure the second scenario should be tested as well.

@jwnimmer-tri
Copy link
Contributor Author

As I understand it, the typical use case is that the user would be loading a blend file in most cases. As such, a regression test with glTF-RPC + blend-preloaded seems like the most important regression test to accomplish.

To help us isolate and debug problems, having a regression test with glTF-RPC only and no blend file is perfectly fine, and might even be the ideal test to add to get us bootstrapped. Or if users would often omit a blend file entirely, then it's a nice regression test to cover them also.

When testing with a blend file, I imagine it should have both lighting settings as well as actual textured geometry. That's a use case we expect users to use, so we need to make sure the images come out correctly in that case.

@jwnimmer-tri
Copy link
Contributor Author

Another thought: we should actually check two RPC requests in a row (using at least slightly different glTF requests). We want to be sure that no state from first request bleeds into the second request's resulting image.

@zachfang
Copy link
Contributor

zachfang commented May 3, 2023

Thoughts after trying to resolve label image differencing.

Context:
As discovered today, the ~5% difference mainly comes from the default RGB values from Blender vs. glTF (or vtkGlTFExporter). Setting the default to (1.0, 1.0, 1.0) seems reasonable, but Blender somehow decides to use (0.8, 0.8, 0.8),

From poking around our Blender server, there is no obvious way, to me, to disable lighting interaction for a textured mesh programmatically. Additionally, if we are targeting a Drake use case, rendering a glTF with textured meshes to a label image is not actually a valid use case, IMO.

Thus, would it make sense to test things separately with slightly different resources while trying to share the common ones?

My current proposal is:

  • A two_diffuse_color_boxes.gltf to test all three image types.
  • A left_diffuse_color_box.gltf and a right_diffuse_color_box_and_light.blend to test all three image types.
  • A one_texture_one_rgba.gltf with an additional color_texture.png to test color rendering.

That would cover all the use cases other than the textured mesh is embedded in the blend file.

@jwnimmer-tri
Copy link
Contributor Author

A two_diffuse_color_boxes.gltf to test all three image types.
A one_texture_one_rgba.gltf with an additional color_texture.png to test color rendering.

Sure, I like these.

A left_diffuse_color_box.gltf and a right_diffuse_color_box_and_light.blend to test all three image types.

When the user is adding a static *.blend file to the scene, I assume they would be using the same file for all of color, label, and depth. And therefore, the file surely would have textures. A blend file with only diffuse does not seem like an interesting / important test case.

On the other hand, I don't think its a big deal? Per RobotLocomotion/drake#18311 the *.blend file all be one label (probably "background"). So either we don't load it when doing label images, or we load it and strip all of the textures off.

@zachfang
Copy link
Contributor

zachfang commented May 3, 2023

What you would suggest to test the blend file loading? Maybe a left_diffuse_color_box.gltf and a right_textured_box_and_light.blend to test only the color (or plus depth)?

@jwnimmer-tri
Copy link
Contributor Author

The end-user story goes like server --blend-file=right_textured_box_and_light.blend. The RPC client should be able to render all three images types from that one server. That's what we should test. So exactly one blend file, with a texture, for all three image types.

The first question is to decide how the server implements label images. Either it needs to not load the blend file in that case, or it needs to somehow disable the blend file. Once you've made that design choice, the unit test can be crafted to match it.

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 a pull request may close this issue.

2 participants