Add no visualization option #2522

aymanhab · 2019-07-10T21:32:24Z

Fixes issue #2375

Brief summary of changes

Introduce a Property in ModelVisualPreferences to turn off loading mesh files.

Testing I've completed

Modified a test case that loads a model and produce missing vtp files warning, then set flag to no_visualization, printed the model. Got no warning on reading model back.

Looking for feedback on...

We could do this without using the Property mechanism by passing an argument to the constructor but that would require more changes to API programs.
Default value was discussed with @jimmyDunne and he prefers default to no visualization.
We could use this flag more widely to generateDecorations but that would be a bigger change.
There's no convenience method to turn visualization back on, can be done through Property methods.

CHANGELOG.md (choose one)

Not modified pending agreement on behavior

The Doxygen for this PR can be viewed at http://myosin.sourceforge.net/?C=N;O=D; click the folder whose name is this PR's number.

This change is

…h loading

…e convenience methods to set it and query it. Use it in Mesh::extendFinalizeFromProperties to avoid file search for mesh files

chrisdembia · 2019-07-10T22:41:39Z

OpenSim/Simulation/Model/ModelVisualPreferences.h

@@ -66,6 +69,7 @@ class OSIMSIMULATION_API ModelVisualPreferences : public Object {
 private:
    void constructProperties() {
        constructProperty_ModelDisplayHints(ModelDisplayHints());
+        constructProperty_no_visualization(false);


We should avoid the possibility for double negatives. I would prefer this setting to be named "visualize" or something like that.

I don't disagree.

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.

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?

@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.

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? If getUseVisualizer is true then it is true. The GUI turns it to true but otherwise it is false.

aseth1 · 2019-07-11T18:22:50Z

OpenSim/Simulation/Model/ModelVisualPreferences.h

@@ -66,6 +69,7 @@ class OSIMSIMULATION_API ModelVisualPreferences : public Object {
 private:
    void constructProperties() {
        constructProperty_ModelDisplayHints(ModelDisplayHints());
+        constructProperty_no_visualization(false);


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? If getUseVisualizer is true then it is true. The GUI turns it to true but otherwise it is false.

aymanhab · 2019-07-11T18:54:19Z

@aseth1 my preference would have been not to use the properties altogether as well, however due to the fact that mesh file loading is performed in extendFinalizeFromProperties which is invoked in the constructor, there's no clear way to pass a flag around to relevant components. The two options were the Properties or to pass a flag to the constructor that's kept around which seemed less friendly to API users. (item 1 in looking for feedback on above).
In essence, by the time you have an instance of the model to setUseVisualizer on you've already loaded the meshes, issued the warnings.

…ity, flip logic and rename methods to reflect feedback on PR.

aymanhab · 2019-07-11T20:50:57Z

I changed the property to 'load_mesh_files' per the feedback so far and flipped the -ve logic. Now pending the discussion of scope. It would be good to run a simulation and place a breakpoint in generateDecorations to see if the method is actually called or only assumed. I can do that myself if no body else did it already.

aseth1 · 2019-07-11T22:30:23Z

I changed the property to 'load_mesh_files' per the feedback so far and flipped the -ve logic. Now pending the discussion of scope.

IMHO a flag to control visualization is better than having to make users aware of loading meshes from files (even though mesh loading is the current issue). It seems like a one line fix to interrupt the delegation to each component in the Model in model's generateDecorations().

aseth1 · 2019-07-11T23:03:47Z

The two options were the Properties or to pass a flag to the constructor that's kept around which seemed less friendly to API users. (item 1 in looking for feedback on above).

Other options are:

do not load meshes in Mesh::extendFinalizeFromProperties and make it use something similar to ContactMesh::createSimTKContactGeometry during adding to the System. Having a single loadMesh mechanism would be nice!
make Mesh::extendFinalizeFromProperties() check Model::visualize() and if 'false' (default) then Mesh ignores loading the mesh file altogether. The driver API (main, simulate, ...) then must set setUseVisualizer (which is already a requirement to visualize via the API) that sets visualize to true which triggers finalizeFromProperties. The GUI would also set visualize to true. This seems pretty easy to do and does not require exposing yet another user property with potential to conflict with an existing flag, e.g: load_meshes == false but getUseVisualizer() == true or vice-versa.

aymanhab · 2019-07-13T01:12:40Z

Some experiments to take into consideration:

testForward with a variety of models never triggered call to generateDecorations anywhere. Is there a reason to think other workflows would trigger it?
Only overhead related to geometry was the calls to extendFinalizeFromProperties which we're optimizing out in this PR.
With the current flag off and useVisualizer on, built in shapes and muscles were all functional and bahaved correctly in the API visualizer so it seems to have the correct granularity and affects only meshes as intended. Same behavior was observed in GUI build out of the box.

aymanhab · 2019-07-16T17:22:18Z

Some more comments:

Moving the code that looks up mesh files somewhere else is very easy (what would be an appropriate place?) the side effects though are hard to predict since there are no automated tests for this. Keep in mind that the calls from model constructor are made with specific current working directory, while calls outside the constructor can be executed in any directory (similar to issues we had dealing with in externalloads with nested files) so I'd prefer to avoid making this change.
Long term plan, from my perspective, is not to use loadMesh altogether for visualization meshes, but rather to move handling of meshes entirely to the GUI visualizer which has more robust parsing and reduces memory footprint (for the application).
Definitely would be great to have a model level flag that explicitly shuts off all visualization but that seems a bit outside the scope of the current issue and requires item 1. above which is a bit risky.

I understand having the property and a call to visualize could possibly be confusing so I'm hoping the current name would help clarify the scope.

…se it before loading mesh files and test it in testVisualization .

aymanhab · 2019-07-18T22:06:31Z

Thanks for the proposal @aseth1 I went ahead and implemented it as you suggested (you can check the branch/PR to make sure we're in sync. documentation may need more work).

The good news is that it worked and the issue with working directory didn't cause a problem at all :+1 Test cases passed except for testActuators which was tripped by the side effect of calling finalizeFromProperties within setUseVisualizer). This however highlights the major drawback that finalizeFromProperties is a massive hammer that wipes out the system, connections and state and recreates them from scratch (rather unnecessarily in this case) just for the side effect of calling extendFinalizeFromProperties on meshes. The GUI will need to be changed to handle that at a dozen places or so where it loads models as well.

Another side effect is that innocent looking calls that used to be sprinkled rather liberally in our code and users code to setUseVisualizer now could have major side-effects (as in testActuators.cpp). Having the API visualizer as a debugging tool that can be used without worry about side-effects is likely very important for developers.

My recommendation at this point would be to stay with the more "proportionate" change that controls meshes only but I'd like to hear your thoughts/feedback and to hear from others @chrisdembia, @tkuchida, @jimmyDunne and @jenhicks Cheers.

aseth1 · 2019-07-18T23:11:55Z

Test cases passed except for testActuators which was tripped by the side effect of calling finalizeFromProperties within setUseVisualizer)

Not a side effect. Calling model.setUseVisualizer() after model.initSystem() is plain wrong. I am surprised this was even running before. The calls to model.setUseVisualizer(false) can simply be removed in the test case. Better yet if the flag is being set to the same value do not call the "hammer".

This however highlights the major drawback that finalizeFromProperties is a massive hammer that wipes out the system, connections and state and recreates them from scratch (rather unnecessarily in this case) just for the side effect of calling extendFinalizeFromProperties on meshes.

This is the designed behavior-- finalizeFromProperties IS supposed to wipe out an existing System since a new System MUST be built to accommodate property changes of all types. In this case, there was a failure to ensure the rather simple sequence of operations: 1) edit the Model as desired including turning visualization flags ON/OFF. 2) then create (i.e. model.initSystem()) the System that represents the Model that has been finalized.

The GUI will need to be changed to handle that at a dozen places or so where it loads models as well.

Then the GUI has been using the API incorrectly, which is unlikely given the series of Exceptions throws to thwart this kind of thing. Meshes should not be loaded on a model with a live System. As properties, this would be equivalent to editing joint locations or spring constants on a model as it is running. Please verify this is not in fact the case.

Another side effect is that innocent looking calls that used to be sprinkled rather liberally in our code and users code to setUseVisualizer now could have major side-effects (as in testActuators.cpp). Having the API visualizer as a debugging tool that can be used without worry about side-effects is likely very important for developers.

Again, the "side-effect" i.e. the failure should happen if the Model is edited. The correct use of setUseVisualizer() as in the vast majority (if not all except testAcuators) will have no issues especially if the value of the flag is not changing. In the case that it is, it will load the meshes as needed, which is what we want!

My recommendation at this point would be to stay with the more "proportionate" change that controls meshes only but I'd like to hear your thoughts/feedback and to hear from others.

You can hear from others, but your description above is distorted. One misuse in a test case does not invalidate the process we have built to ensure that our simulations are valid. I believe the fix works as expected and the test case can be repaired by deleting two lines of code (which were incorrect usage in the first place).

aseth1

For testActuators to pass, remove the two instances where model.setUserVisualizer(false) is called after model.initSystem(). This is effectively a property change.
To mitigate unnecessary invocations of finalizeFromProperties do not invoke if the visualization flag is unchanged from its present value.

aseth1 · 2019-07-18T22:55:06Z

OpenSim/Simulation/Model/Model.h

+    void enableVisualization(bool enable) {
+        // Propagate flag so that meshes are loaded (or not)
+        upd_ModelVisualPreferences().setVisualize(enable);
+        finalizeFromProperties();


If you want this to be a bit more efficient, you could check that the enable status is changing. If not, no reason to do anything.

aymanhab added 2 commits July 9, 2019 15:38

Add no_visualization Property to Model class and use it to bypass mes…

471c67c

…h loading

Create no_visualization Property in ModelVisualPreferences and provid…

74dd41f

…e convenience methods to set it and query it. Use it in Mesh::extendFinalizeFromProperties to avoid file search for mesh files

chrisdembia reviewed Jul 10, 2019

View reviewed changes

aseth1 requested changes Jul 11, 2019

View reviewed changes

Change property name to load_mesh_files to reflect current functional…

1e6278f

…ity, flip logic and rename methods to reflect feedback on PR.

Merge branch 'master' into add_no_visualization_option

fdf92e4

aymanhab added 2 commits July 16, 2019 15:32

Remove property and introduce flag to enable/disable visualization, u…

7127e6f

…se it before loading mesh files and test it in testVisualization .

Key off Model's new isVisualizationEnabled method to load mesh files

f1bba44

aseth1 requested changes Jul 18, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no visualization option #2522

Add no visualization option #2522

aymanhab commented Jul 10, 2019 •

edited

Loading

chrisdembia Jul 10, 2019

tkuchida Jul 10, 2019

aymanhab Jul 11, 2019

tkuchida Jul 11, 2019

aymanhab Jul 11, 2019

aseth1 Jul 11, 2019 •

edited

Loading

aseth1 Jul 11, 2019 •

edited

Loading

aymanhab commented Jul 11, 2019

aymanhab commented Jul 11, 2019

aseth1 commented Jul 11, 2019

aseth1 commented Jul 11, 2019

aymanhab commented Jul 13, 2019 •

edited

Loading

aymanhab commented Jul 16, 2019

aymanhab commented Jul 18, 2019

aseth1 commented Jul 18, 2019

aseth1 left a comment

aseth1 Jul 18, 2019

Add no visualization option #2522

Are you sure you want to change the base?

Add no visualization option #2522

Conversation

aymanhab commented Jul 10, 2019 • edited Loading

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

chrisdembia Jul 10, 2019

Choose a reason for hiding this comment

tkuchida Jul 10, 2019

Choose a reason for hiding this comment

aymanhab Jul 11, 2019

Choose a reason for hiding this comment

tkuchida Jul 11, 2019

Choose a reason for hiding this comment

aymanhab Jul 11, 2019

Choose a reason for hiding this comment

aseth1 Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

aseth1 Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

aymanhab commented Jul 11, 2019

aymanhab commented Jul 11, 2019

aseth1 commented Jul 11, 2019

aseth1 commented Jul 11, 2019

aymanhab commented Jul 13, 2019 • edited Loading

aymanhab commented Jul 16, 2019

aymanhab commented Jul 18, 2019

aseth1 commented Jul 18, 2019

aseth1 left a comment

Choose a reason for hiding this comment

aseth1 Jul 18, 2019

Choose a reason for hiding this comment

aymanhab commented Jul 10, 2019 •

edited

Loading

aseth1 Jul 11, 2019 •

edited

Loading

aseth1 Jul 11, 2019 •

edited

Loading

aymanhab commented Jul 13, 2019 •

edited

Loading