-
Notifications
You must be signed in to change notification settings - Fork 328
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 no visualization option #2522
Open
aymanhab
wants to merge
6
commits into
main
Choose a base branch
from
add_no_visualization_option
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
471c67c
Add no_visualization Property to Model class and use it to bypass mes…
aymanhab 74dd41f
Create no_visualization Property in ModelVisualPreferences and provid…
aymanhab 1e6278f
Change property name to load_mesh_files to reflect current functional…
aymanhab fdf92e4
Merge branch 'master' into add_no_visualization_option
aymanhab 7127e6f
Remove property and introduce flag to enable/disable visualization, u…
aymanhab f1bba44
Key off Model's new isVisualizationEnabled method to load mesh files
aymanhab File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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.
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.
We should avoid the possibility for double negatives. I would prefer this setting to be named "visualize" or something like that.
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 don't disagree.
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 made sure the user facing convenience method name is clear, I definitely welcome better names for the property that will appear in xml and will flip the logic accordingly. I don't particularly like visualize as it is close to "visualizer" so rather overused, but I welcome other opinions.
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 description is "Flag to indicate whether or not to show geometry from mesh files, default to false." so I assume analytic geometry would still visualize. If that's the case, then perhaps put "Mesh" in the name of the property, like
loadMeshFiles
?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.
@tkuchida I would have called it show_meshes but I wanted to get feedback on (item 3) whether we want to use this more widely to avoid calling generateDecorations altogether which would affect all geometry but may be more efficient.
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 seems somewhat confusing to me particularly in context of existing
Model::get/setUseVisualizer()
. I realize that this is for the API visualizer and not the GUI visualizer. If you are not using the API visualizer (and you are not in the GUI) you would expect zero overhead/errors related to visualization. If you are in the GUI or have the visualizer on, then visualize should be true. You shouldn't have to turn two flags on.I like the idea of bypassing
generateDecorations()
altogether if there is no visualization.Can we simplify the interface and only have one internal
visualize
flag without exposing any properties? IfgetUseVisualizer
is true then it is true. The GUI turns it to true but otherwise it is false.