Add testing for human rendering, default_cam_config option in kwargs for customization #94
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…for that and other human rendering
| "distance,elevation", | ||
| [(2.0, -45), (10.0, -45), (1.0, -45), (2.0, 45), (10.0, 45), (1.0, 45)], | ||
| ) | ||
| def test_render_human_default_cam_config(distance, elevation): |
There was a problem hiding this comment.
Im a bit confused by this test, what are we testing and how?
We are not testing that any value is equal to something or an event happens.
Just that some code runs with particular kwargs, no where testing that this does an expected thing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses an issue raised in #90
We could also add something that tests video recording like in #15 just to ensure things still work, but probably not as important (ensuring human rendering works in this PR is much more important imo).
Only downside is some things need manual validation to see if the rendering kwargs actually do the right thing, and if the scene is frozen, but at the very least this ensures that the human rendering does work and that the args go through correctly, and makes it extremely easy for us to go through and check that things work (for example for troubleshooting the gymnasium mujoco rendering issue we hypothesized in that issue)
Got a little distracted digging into the mujoco rendering and then saw that you can specify the default_cam_config, so I tried that and thought well this is probably something users would want to be able to control, so I added that as well. Fixed a few little nitpicks like missing periods in the docstrings as well.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue), Depends on # (pull request)
Type of change
Screenshots
Checklist:
pre-commitchecks withpre-commit run --all-files(seeCONTRIBUTING.mdinstructions to set it up)pytest -vand no errors are present.pytest -vhas generated that are related to my code to the best of my knowledge.