Skip to content
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

Don't prompt to save changes when only coordinates are changed #783

Open
tgeijten opened this issue Sep 14, 2023 · 7 comments
Open

Don't prompt to save changes when only coordinates are changed #783

tgeijten opened this issue Sep 14, 2023 · 7 comments

Comments

@tgeijten
Copy link

I often use OSC to inspect an existing model by using the coordinate sliders to change the pose. When I then close the model, OSC always prompts to save the model, which is never what I want. It's quite easy to miss the 'no' button and unintentionally overwrite the model with a new version, which can be quite dangerous, especially if you're using 'standard' published models.

Is it possible to not regard pose changes as changes to the model -- unless the user explicitly presses a "Save default pose" button in the coordinates window?

@adamkewley
Copy link
Collaborator

Duplicate: #412 (semi-related to a suggestion you made w.r.t. muscle plotting).

I understand that, if coming from OpenSim, it's potentially confusing behavior to handle coordinate edits as model edits. However, my opinion on the matter is that your desired behavior is not obvious to users that are unfamiliar with OpenSim. This is why changing it all-in (with no toggle, etc.) has been kicked back, but I'm willing to consider making this a toggle-able behavior.

I would guess that users expect when they press save in an editing UI that all visible edits are saved, the software can be safely closed, and the original workspace can be reproduced by reopening the saved file. I would also guess that your concern (paraphrasing: "it is quite dangerous that I could accidentally save a pose change") is either:

  • Roughly as concerning as the statement: "I could accidentally save my Word/Blender/Photoshop/Inkscape/Source Code/Kdenlive/Godot/Unity/etc. document after I edit it"
  • A legitimate concern from (specifically) OpenSim users. OSC tries to counter this by only prompting to save when an edit (incl. pose) is made, which is a common UI idiom from other software.

E.g. when we teach undergraduates the OpenSim GUI, one of the most common mistakes is that they think coordinate edits are part of their model edits. They usually do something like (e.g.) pose a model, go "this looks good", hit save, and then use the model in a MATLAB script or similar to produce a plot.

My counter-argument is that it's more dangerous for an editor to silently drop certain types of edits and expect users to know which ones.


To resolve your concern, I would suggest this compromise:

  • Internally, I'll re-separate poses from OpenSim::Model in the datastructures that OSC handles
  • But, by default, keep the behavior as it is currently is
  • And provide a configuration option that enables OpenSim-like "the pose is separate from the model" behavior. Perhaps even exposing this as a high-level use_opensim_style_edits configuration option that configures a wider range of tweaks would be useful.

This can then be rolled out and tested by you, and then exposed via the UI, or as a first-boot option (e.g. "choose what style of editing you want"), and if the greater user-base want it as the default then it becomes a matter of setting something somewhere to true, rather than there being a flip-flopping in the codebase itself.

(but, I am willing to bet, most non-OpenSim users--OSC's target audience--will prefer the editor-behaves-as-any-other-editor behavior).

@tgeijten
Copy link
Author

Please allow me to explain my use case a little better. Most of the time I'm using OSC is when I'm inspecting existing models, such as the Rajagopal or Delp model. I never ever intend to modify any these models and save them under the same name, because they are published models and their originals should always be preserved (having slightly modified versions with the same name leads to all kinds of nasty stuff). The only thing I wish for is e.g. to plot some moment arm graphs for some specific muscles, because OSC is really great for that. I do not consider plotting graphs and interacting with them to be an edit -- I'm just exploring the models.

Roughly as concerning as the statement: "I could accidentally save my Word/Blender/Photoshop/Inkscape/Source Code/Kdenlive/Godot/Unity/etc. document after I edit it"

For me the analogy would really be: "I'm browsing through a PDF, and when I close it I'm asked to save my changes because the active page has changed."

Another important consideration: because of rounding errors (I presume), not only the pose is changed when you save in OSC, but also the many other values -- effectively changing the entire model even if you revert to the original pose. Try opening Rajagopal, do a direct "Save As", and diff the two files. The many changes you'll see which will cause any predictive simulations to generate different results even if the simulation doesn't depend on the initial pose. Nasty stuff :-)

An option would be fine of course, but I really wanted to present my point of view and argue that it really has nothing to do with "being used to OpenSim behavior". I hope this makes sense!

@adamkewley
Copy link
Collaborator

Your response is fair, but I have to point out:

I never ever intend to modify any these models and save them under the same name, because they are published models and their originals should always be preserved.

From what I understand, your workflow specifically is:

  • You want to plot things in an existing model.
    • Note: plotting, watching outputs, etc. should not modify your model in any way in OSC: that's a bug if it happens.
  • You also want to change something in the model - specifically, coordinates.
  • And that change should have all the side-effects of any other type of change (e.g. all 3D renders are updated, all plots are updated, undo/redo works as expected, etc.)
  • But that specific change should be treated as a non-edit because you know that, internally, OpenSim may represent those edits separately via a SimTK::State
  • And a primary motivator for wanting this is because it's possible to accidentally press Yes in response to the "Do you want to save changes?" dialog
  • And, as a separate concern, re-saving a file from OpenSim may introduce rounding errors

I think we just have a difference of opinion from a design PoV. Yes, OSC has some plotting/output capabilities--and will likely have more in the future--but the primary motivation of OSC is to create/edit models; therefore, the primary design philosophy centers around OSC having similar behavior to other common editing software. It's just that it also tries to be similiar-enough to OpenSim GUI so that those users feel like it's a familiar tool - although, having configuration options to narrow the gap even more is also a good idea.


"I'm browsing through a PDF, and when I close it I'm asked to save my changes because the active page has changed."

This highlights our difference of opinion. To you, editing a coordinate is, in a way, in the same category as scrolling through a PDF, or moving a 3D camera, or changing a viewport overlay. To me, editing a coordinate is, in a way, almost exactly the same as editing anything else in the model because it clearly changes the model in all viewports, changes outputs, changes simulation results, etc.


Again, I'm willing to consider perhaps making it a configuration option. But engineering it (either way), is going to take time because it means that the entire UI has to drag around (model, possibleStateEditsThatMightBeSavedMaybeOnUserRequest) rather than just (model). This is non-trivial because OSC's an edit-heavy UI with a growing amount of widgets that do things like automatically update, track changes, etc.

@tgeijten
Copy link
Author

tgeijten commented Sep 14, 2023

note from @adamkewley - I accidently edited over the original version then reverted this post: the formatting isnt' @tgeijten 's

  • You want to plot things in an existing model.
    • Note: plotting, watching outputs, etc. should not modify your model in any way in OSC: that's a bug if it happens.
  • You also want to change something in the model - specifically, coordinates.

Interacting with a plot changes the coordinates. The interactive plots are a really powerful feature of OSC, because they allow you to see how moment arms etc. relate to pose.

  • And that change should have all the side-effects of any other type of change (e.g. all 3D renders are updated, all plots are updated, undo/redo works as expected, etc.)
  • But that specific change should be treated as a non-edit because you know that, internally, OpenSim may represent those edits separately via a SimTK::State
  • And a primary motivator for wanting this is because it's possible to accidentally press Yes in response to the "Do you want to save changes?" dialog
    -And, as a separate concern, re-saving a file from OpenSim may introduce rounding errors

That pretty much sums it up. Only the way you phrase it makes it seem like it's a very specific and niche request, while to me it seems like a super common use case.

"I'm browsing through a PDF, and when I close it I'm asked to save my changes because the active page has changed."

This highlights our difference of opinion. To you, editing a coordinate is, in a way, in the same category as scrolling through a PDF, or moving a 3D camera, or changing a viewport overlay. To me, editing a coordinate is, in a way, almost exactly the same as editing anything else in the model because it clearly changes the model in all viewports, changes outputs, changes simulation results, etc.

You could run a poll with biomechanists and see what they think.

Again, I'm willing to consider perhaps making it a configuration option. But engineering it (either way), is going to take time because it means that the entire UI has to drag around (model, possibleStateEditsThatMightBeSavedMaybeOnUserRequest) rather than just (model). This is non-trivial because OSC's an edit-heavy UI with a growing amount of widgets that do things like automatically update, track changes, etc.

Can't it just be a global setting?

@adamkewley
Copy link
Collaborator

adamkewley commented Sep 14, 2023

That pretty much sums it up. Only the way you phrase it makes it seem like it's a very specific and niche request, while to me it seems like a super common use case.
...
You could run a poll with biomechanists and see what they think.

Probably the difference between our two backgrounds. I would say, as a person who's worked with a lot of artistic/editing software over the years, that it's a niche request, yes. Not that it's a bad request--biomechanics is a niche, after all--but I'm merely spelling out exactly what you want in the language of other editing softwares' designs.

I'd be happy to poll it if there is an unbiased source of people to poll (e.g. students, new PhDs, prospective users who aren't using OpenSim yet). However, I fear that the easiest community for me to poll (the OpenSim community) also is the most likely to say "no changes", because that is exactly what one would expect.

Can't it just be a global setting?

The setting is the easy part. The implementation is where things get sticky. E.g. the implementation has to:

  • Supply OpenSim::Model const& on-demand
  • Supply SimTK::State const& on-demand
  • Provide a way to edit the model in a scratch space (OpenSim::Model&)
  • Create a brand new SimTK::State after every model edit (problematic for this proposal)
  • So, provide a way to edit your concerns (e.g. hold CoordinateEdits next to the model)
  • And change any widgets that specifically want to edit those concerns, rather than the model itself
  • And store those separate concerns alongside the model in undo/redo history
  • And garbage-collect those concerns after every model-edit, because their associative nature means that a model edit may invalidate the concern (e.g. delete joint, add joint, change joint, change permitted range of joint, etc.)
  • And provide a method for hard-applying the concerns to the model when the user specifically requests that they want the current pose set as default (in OpenSim GUI terms)
  • And ensure that editing those concerns affect cached IDs and metadata, such as model/state version, etc. correctly, so that the various widgets that are polling for changes get an answer that's accurate for their level of concern

It's more complicated than SCONE/OpenSim GUI may make it seem because the range of possible edits is wider - and widening with each feature addition.

@tgeijten
Copy link
Author

Probably the difference between our two backgrounds. I would say, as a person who's worked with a lot of artistic/editing software over the years, that it's a niche request, yes. Not that it's a bad request--biomechanics is a niche, after all--but I'm merely spelling out exactly what you want in the language of other editing softwares' designs.

I have a background in artistic editing software as well, long before I started doing biomechanical modeling. In the end, all software are just tools to accomplish specific goals. 'Design language' is only useful to the extend it helps accomplish those goals.

I'd be happy to poll it if there is an unbiased source of people to poll (e.g. students, new PhDs, prospective users who aren't using OpenSim yet). However, I fear that the easiest community for me to poll (the OpenSim community) also is the most likely to say "no changes", because that is exactly what one would expect.

I think when you're developing software for biomechanical modeling it's a good idea to ask people who do biomechanical modeling.

Also, most biomechanists I know who use OpenSim are craving for something more user-friendly and don't care about OpenSim-specific ways of doing things.

The setting is the easy part. The implementation is where things get sticky. E.g. the implementation has to:

  • Supply OpenSim::Model const& on-demand
  • Supply SimTK::State const& on-demand
  • Provide a way to edit the model in a scratch space (OpenSim::Model&)
  • Create a brand new SimTK::State after every model edit (problematic for this proposal)
  • So, provide a way to edit your concerns (e.g. hold CoordinateEdits next to the model)
  • And change any widgets that specifically want to edit those concerns, rather than the model itself
  • And store those separate concerns alongside the model in undo/redo history
  • And garbage-collect those concerns after every model-edit, because their associative nature means that a model edit may invalidate the concern (e.g. delete joint, add joint, change joint, change permitted range of joint, etc.)
  • And provide a method for hard-applying the concerns to the model when the user specifically requests that they want the current pose set as default (in OpenSim GUI terms)
  • And ensure that editing those concerns affect cached IDs and metadata, such as model/state version, etc. correctly, so that the various widgets that are polling for changes get an answer that's accurate for their level of concern

It's more complicated than SCONE/OpenSim GUI may make it seem because the range of possible edits is wider - and widening with each feature addition.

That is not a good sign.

@adamkewley
Copy link
Collaborator

adamkewley commented Sep 14, 2023

That is not a good sign

Unfortunately, it's what is probably necessary to implement an editor that's robust enough to withstand a wide variety of edits via the OpenSim API without segfaulting or entering unusual UI states.

The story behind it is that the primary datastructure (Model) both creates and indexes into the secondary datastructure (State), but Model edits can change the layout of State, which other UI editors tend to overlook (e.g. because changing a property on an existing component tree wouldn't change the layout of the state). So, to be safe, OSC nukes State every time Model changes so that it "knows" the two are safe to pair together.

However, this nuking process creates a necessity for a 3rd runtime-checked associative datastructure (CoordinateEdits/StateEdits) to safely add any not-model-level edits. But, because that datastructure is associative and (for sanity's sake) isn't manually updated whenever the model is edited, it requires GCing etc. after every edit, and it requires that the UI talks to it whenever it wants to make an edit that isn't part of the model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants