-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Test and check for texture randomization #1705
base: feature/texture-randomization
Are you sure you want to change the base?
Test and check for texture randomization #1705
Conversation
|
||
# check to make sure replicate_physics is set to False, else raise warning | ||
if replicate_physics: | ||
raise Warning("replicate_physics is set to True, meaning all environments will have same textures applied.") |
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.
This is never desired. You should throw a runtime error and tell the user to set this parameter as false in the InteractiveSceneCfg
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.
Do you mean we should change this to an error just in here? Or do you mean we should move the error to within the InteractiveSceneCfg?
@@ -233,6 +236,7 @@ def __call__( | |||
env_ids: torch.Tensor, | |||
event_name: str, | |||
asset_cfg: SceneEntityCfg, | |||
replicate_physics: bool, |
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.
Why is this parsed here? You should get it from the scene, right?
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.
fixed, now referenced directly from the scene
) | ||
|
||
|
||
def main(): |
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.
This isn't a unit test. We should do it properly to check that texture is applied on the prim (besides visual inspection)?
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.
The initial idea was to check the textures being applied changed on reset for the prims in the scene, however I was unable to find a way to directly access the albedo map information for each prim. If you have a way to access this information or a different approach I can convert this into a unit test.
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.
@hapatel-bdai let's take a look at this and figure out how to verify the textures for a little today / tomorrow
If we can't figure out how to check that the correct texture was applied, we can have a test_texture_randomization.py
that is similar to this check
script (only it needs to terminate). This will at least give us confidence that this codepath doesn't throw exceptions
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.
Overall, LGTM, we just need to figure out the unit test part. Great work!
@@ -48,6 +48,7 @@ Guidelines for modifications: | |||
* Gary Lvov | |||
* Giulio Romualdi | |||
* Haoran Zhou | |||
* Harsh Patel |
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.
🎉
@@ -196,6 +196,10 @@ def __init__(self, cfg: EventTermCfg, env: ManagerBasedEnv): | |||
event_name = cfg.params.get("event_name") | |||
texture_rotation = cfg.params.get("texture_rotation", (0.0, 0.0)) | |||
|
|||
# check to make sure replicate_physics is set to False, else raise warning | |||
if env.cfg.scene.replicate_physics: | |||
raise Warning("replicate_physics is set to True, meaning all environments will have same textures applied.") |
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.
I think what Mayank is suggesting is:
raise Warning("replicate_physics is set to True, meaning all environments will have same textures applied.") | |
raise ValueError("Unable to randomize visual texture material - ensure InteractiveSceneCfg's replicate_physics parameter is set to False.") |
I don't think we can move the error the InteractiveSceneCfg
, because at that point we don't know the user wants to do scene randomization
Also, TIL about python's Warning
class?!?
# add argparse arguments | ||
parser = argparse.ArgumentParser(description="Check applying texture randomization to the cartpole scene.") | ||
parser.add_argument("--num_envs", type=int, default=16, help="Number of environments to spawn.") | ||
parser.add_argument( |
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.
NIT: I might mention here that this should be set to False, but is left as a CLI arg for testing purposes?
parser = argparse.ArgumentParser(description="Check applying texture randomization to the cartpole scene.") | ||
parser.add_argument("--num_envs", type=int, default=16, help="Number of environments to spawn.") | ||
parser.add_argument( | ||
"--replicate_physics", type=bool, default=False, help="Replicates physics properties across all environments." |
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.
"--replicate_physics", type=bool, default=False, help="Replicates physics properties across all environments." | |
"--replicate_physics", type=bool, default=False, help="Replicates physics properties across all environments. For texture randomization, it must be set to False." |
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.
Made the changes suggested above, we can go over them and the unit test situation tomorrow :)
) | ||
|
||
|
||
def main(): |
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.
@hapatel-bdai let's take a look at this and figure out how to verify the textures for a little today / tomorrow
If we can't figure out how to check that the correct texture was applied, we can have a test_texture_randomization.py
that is similar to this check
script (only it needs to terminate). This will at least give us confidence that this codepath doesn't throw exceptions
Description
Added a visual test of the texture randomization functionality working
Type of change
Checklist
pre-commit
checks with./isaaclab.sh --format
config/extension.toml
fileCONTRIBUTORS.md
or my name already exists there