Update bouncing_block.osim geometry

[aseth1]

The original custom made geometry (.vtp) files: big_block_centered.vtp and linkage1.vtp are binary formats that are not currently supported in 4.0. Rather than convert these to be ascii, I replaced them with already available block and sphere .vtp files that are available in 3.3 and 4.0.

This update was required for a fix to the related API issue API 1762 and subsequent PR 1902.

I am able to load these models in both 3.3 and 4.0 without issues.

[tkuchida]

Two questions: 1. How do I review PRs in this repo? I don't see a README, CONTRIBUTING, etc. Do I need to pull opensim-core master and open there, or use the most recent GUI build? 2. Do we want to preserve the original version (20303) for our (eventual) updateFromXMLNode tests?

[aseth1]

@tkuchida good questions.

1. I used the most recent GUI build, at the time, and I think that is the best way to verify that users can load the models going forward. The CI test will load and invoke initSystem(), which should verify that the model is loaded without errors in the latest version of the API (opensim-core), so reviewers don't have to do that.
2. We definitely want to test updating from pre-4.0 models to 4.0, but since 3.3 was already updating to older versions to 30000, I think it is OK for any update from XML testing to focus on 3.x to 4.0 conversions.

This PR is just a fix to get around the inability of 4.0 to load binary vtp files. It would be good to establish rules for contributing models to this repo (especially as it is now public) and we should discuss that in a future dev meeting. Thank you for raising it!

[chrisdembia]

We discussed reviewing rules a few weeks ago and I think we said that we should follow the same rules for this repo as for core (2 reviewers, etc.). I asked @jimmyDunne if he wanted to always be one of the reivewers and he said no; it's fine to merge a PR even if he has not reviewed it.

[aseth1]

Thanks @chrisdembia. I was also wondering if we should have criteria for contributions. E.g. models must have a permissive license (not for academic use only); meet a need that is not currently met by existing models in the repo (to limit having 20 gait models); and should have a unique research or pedagogical purpose (again so we don't have 10 different types of bouncing boxes!), ... What do you think? @opensim-org/dev-team

[tkuchida]

We definitely want to test updating from pre-4.0 models to 4.0, but since 3.3 was already updating to older versions to 30000, I think it is OK for any update from XML testing to focus on 3.x to 4.0 conversions.

This statement conflicts with what I interpret to be the spirit of 206 on opensim-core. It could be particularly important to preserve old files for testing if updateFromXMLNode() is changed to convert a model one full version at a time (rather than one node at a time, regardless of version); see 1588 on opensim-core. I suppose we could ignore for now and exhume old file versions if necessary.

[aymanhab]

Can I merge this in now? @tkuchida

[tkuchida]

Can I merge this in now?

Presumably, assuming my interpretation of 206 on opensim-core is incorrect.

[aseth1]

@tkuchida I don't think your interpretation of 206 on opensim-core is incorrect, but the this models repo shouldn't have to maintain old files. I like your proposal on core (in 206) to have separate suffixes or folders to contain files from older versions of OpenSim for the purpose of testing conversion. Those models could live in shared testing area on core.

[tkuchida]

@aseth1 I understand that this is the models repo. I believe this PR will replace model files with new versions, so unless we archive the current files before merging, we will need to dig through the repo history if we want the old versions---which is also fine I guess.

[aseth1]

I believe this PR will replace model files with new versions, so unless we archive the current files before merging

Good point. The 20303 model will not load in 4.0 because of the geometry, so archiving it (without the fix) won't be very useful. Surgically modifying the original 20303 format might be easiest - which was your original suggestion. Looking at the diff it seems straightforward though.

[aseth1]

@tkuchida @aymanhab I reverted the update to 30000 back to 20303 so that the changes are strictly to the geometry. Both models loads and views in 3.3 (GUI) and 4.0 (master->visualizeModel) and 4.0Beta_111417, but when I simulate in the latter with the weak spring the resulting animation inconsistent with the simulated results when plotted.

@aymanhab any ideas what is happening here? I don't think the bug is specific to the model. I do see an assembly failure in 4.0 to meet required accuracy. Relaxing the accuracy makes the error go away but the visualization is the same.

[tkuchida]

👍

[aymanhab]

@aseth1 Does the model have constraints? that may explain the assembly issue, I believe when we animate we blast the state and do not assemble.
Do the models show the same initially and with slider moves? Could it be that the assumptions made by the idealistic cube/box are different from the file based cube (origin, axes etc.?)

[aseth1]

@aymanhab yes there is a constraint and there was an assembly failure immediately following forward integration. The results (coordinates above) reported are correct, so if they were "blasted" to screen it should be OK. It seems like the simulation is fine, but the state used to draw is wrong, but I have no way of accessing that. How do the coordinates in the results motion differ from what the GUI uses to draw? Also the coordinate sliders reflect the state used to draw and not the stated computed. Any ideas why the coordinates in the results differ from what is being displayed in both the coordinates sliders and the visualization?

I am certain it has nothing todo with the display geometry, because running the model with high stiffness the results are identical between 3.3 and 4.0. Somehow the low stiffness case is triggering a mismatch in what was computed and what is being displayed.

[aseth1]

@aymanhab this PR should have been merged ages ago. The model does not load on 4.0 on Windows.
The difference in visualization is a 4.0 GUI bug that enforces the constraints on replay which are not enforced during forward simulation.

[aseth1]

Visualization of simulation results is a separate issue , I am merging so that there is a chance to get the correct model in for the testing session tomorrow.