ComputationalBiomechanicsLab / opensim-creator

A UI for building OpenSim models
https://opensimcreator.com
Apache License 2.0
142 stars 17 forks source link

Remember muscle styling options #782

Closed tgeijten closed 1 year ago

tgeijten commented 1 year ago

Users now have to set their prefered option every time model is loaded. Remembering the settings could be automatic, or there could be a "Use current settings as default" button in the muscle styling menu.

adamkewley commented 1 year ago

I agree that this is annoying

The reason it's so is because OSC doesn't currently have a runtime-updateable configuration file. I've been kicking it back for a while--I didn't want to commit to a particular configuration structure--but I should probably go ahead and spec one out (and handle issues like partial writes, parsing errors, forward compatability, etc.)

It's common for users to customize their visualizer settings with stuff like "no floor" and "this bg color", so I should prioritize it

tgeijten commented 1 year ago

Can’t you just hijack Dear ImGui settings system? :-)

From: Adam Kewley @.> Sent: 2023-09-14 9:38 To: ComputationalBiomechanicsLab/opensim-creator @.> Cc: Thomas Geijtenbeek @.>; Author @.> Subject: Re: [ComputationalBiomechanicsLab/opensim-creator] Remember muscle styling options (Issue #782)

I agree that this is annoying

The reason it's so is because OSC doesn't currently have a runtime-updateable configuration file. I've been kicking it back for a while--I didn't want to commit to a particular configuration structure--but I should probably go ahead and spec one out (and handle issues like partial writes, parsing errors, forward compatability, etc.)

It's common for users to customize their visualizer settings with stuff like "no floor" and "this bg color", so I should prioritize it

— Reply to this email directly, view it on GitHub https://github.com/ComputationalBiomechanicsLab/opensim-creator/issues/782#issuecomment-1718923623 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6V4TNUBHLX6IM5ER5CHELX2KX6HANCNFSM6AAAAAA4XTVYNE . You are receiving this because you authored the thread. https://github.com/notifications/beacon/AC6V4TO7XDSFTA7UUUOXL63X2KX6HA5CNFSM6AAAAAA4XTVYNGWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTGOSYWO.gif Message ID: @. @.> >

adamkewley commented 1 year ago

Can’t you just hijack Dear ImGui settings system? :-)

Technically, but I'd prefer to not to couple OSC's configuration system with the internals of one of its UI libraries. ImGui's using its own INI format (probably parseable as toml, but who knows), and ImGui's configuration contains hundreds of lines related to panel positions, docking, etc.

I'd prefer to have a fully designed OSC configuration file that uses a standard format (e.g. toml), only contains options that typical users will want to see/edit, has clear defaults/behavior, etc.

adamkewley commented 1 year ago

The underlying architecture for getting/setting user-enacted configuration options is now in place, it now needs to be hooked up to the relevant UI datastructures (e.g. osc::MuscleDecorationStyle)

adamkewley commented 1 year ago

The UI now detects when a user changes one of these settings, the next challenge is to take each (binary) setting element in osc::ModelRendererParams and figure out a way to (de)serialize it from the osc::AppSettings at the right time.

The following keys need to be handled on a per-viewer basis:

- panels/viewer0/decorations/muscle_decoration_style
- panels/viewer0/decorations/muscle_coloring_style
- panels/viewer0/decorations/muscle_sizing_style
- panels/viewer0/decorations/show_scapulothoracic_joints
- panels/viewer0/decorations/show_muscle_origin_effective_line_of_action
- panels/viewer0/decorations/show_muscle_insertion_effective_line_of_action
- panels/viewer0/decorations/show_muscle_origin_anatomical_line_of_action
- panels/viewer0/decorations/show_muscle_insertion_anatomical_line_of_action
- panels/viewer0/decorations/show_centers_of_mass
- panels/viewer0/decorations/show_point_to_point_springs
- panels/viewer0/decorations/show_contact_forces

- panels/viewer0/overlays/draw_xz_grid
- panels/viewer0/overlays/draw_xy_grid
- panels/viewer0/overlays/draw_axis_lines
- panels/viewer0/overlays/draw_aabbs
- panels/viewer0/overlays/draw_bvh

- panels/viewer0/graphics/draw_floor
- panels/viewer0/graphics/draw_mesh_normals
- panels/viewer0/graphics/draw_shadows
- panels/viewer0/graphics/draw_selection_rims

- panels/viewer0/light_color
- panels/viewer0/background_color
- panels/viewer0/floor_location

(ignore camera params)
adamkewley commented 1 year ago

The implementation now writes (no reading yet) user-enacted viewer settings changes to the user configuration.

Example:

# configuration options
#
# you can manually add config options here: they will override the system configuration file, e.g.:
#
#     initial_tab = "LearnOpenGL/Blending"
#
# beware: this file is overwritten by the application when it detects that you have made changes

[panels.viewer0]
muscle_coloring_style = 'opensim'
muscle_decoration_style = 'hidden'
muscle_sizing_style = 'pcsa_derived'
adamkewley commented 1 year ago

The implementation now reads and writes user-enacted changes to the decoration settings.

These only apply for:

Todo:

adamkewley commented 1 year ago

The implementation now handles all user-interactable flags/options, producing a file like this when the application is closed:

[panels.viewer0]
is_enabled = true

    [panels.viewer0.decorations]
    muscle_coloring_style = 'opensim'
    muscle_decoration_style = 'opensim'
    muscle_sizing_style = 'opensim'
    should_show_scapulo = false
    show_centers_of_mass = false
    show_contact_forces = false
    show_muscle_insertion_anatomical_line_of_action = false
    show_muscle_insertion_effective_line_of_action = true
    show_muscle_origin_anatomical_line_of_action = false
    show_muscle_origin_effective_line_of_action = false
    show_point_to_point_springs = true

    [panels.viewer0.graphics]
    show_floor = false
    show_mesh_normals = false
    show_selection_rims = true
    show_shadows = true

    [panels.viewer0.overlays]
    show_aabbs = false
    show_axis_lines = false
    show_bvh = false
    show_xy_grid = false
    show_xz_grid = true
    show_yz_grid = false

(note: these are only written if the user interacts with that toggle in the UI - otherwise, it's assumed that the user is happy with the system default)

The remaining steps are:


Seperately:

adamkewley commented 1 year ago

After an inordinate amount of what-happens-when-values-are-converted-from-this-or-that-string-format faffing around, the configuration API now supports setting/getting osc::Color values, which means that OSC can then write/read them as hex strings:

[panels.viewer0]
background_color = '#262626ff'
is_enabled = true
light_color = '#ffffffff'

    [panels.viewer0.decorations]
    muscle_coloring_style = 'opensim'
    muscle_decoration_style = 'opensim'
    muscle_sizing_style = 'opensim'
    should_show_scapulo = false
    show_centers_of_mass = false
    show_contact_forces = false
    show_muscle_insertion_anatomical_line_of_action = false
    show_muscle_insertion_effective_line_of_action = true
    show_muscle_origin_anatomical_line_of_action = false
    show_muscle_origin_effective_line_of_action = false
    show_point_to_point_springs = true

    [panels.viewer0.graphics]
    show_floor = false
    show_mesh_normals = false
    show_selection_rims = true
    show_shadows = true

    [panels.viewer0.overlays]
    show_aabbs = false
    show_axis_lines = false
    show_bvh = false
    show_xy_grid = false
    show_xz_grid = true
    show_yz_grid = false

I'm going to stop short of implementing glm::vec3 support for now, because it involves implementing support for list-types in the settings API, plus conversion rules (e.g. if the settings contains a list, and the list is three floating-point numbers, then the setting value can be converted into a glm::vec3).


So the only remaining steps here (ignoring the cleanup, which will be launched into a Refactor... ticket), are to ensure that these settings are used by the simulation tabs but aren't used by other workflows.

adamkewley commented 1 year ago

Edge case found: multi-tab synchronization

If both an editor and a simulator tab are open at the same time, and they share visualizer configurations (in this case, they do), then closing (e.g.) a simulator tab will save the simulator's latest state but will that won't be reflected in the editor tab.

adamkewley commented 1 year ago

This is now "done enough", and can probably be released as is.

There's some outstanding bugs/work-tasks related to this that I will break out into smaller issues that can be chipped away at over future releases, rather than allowing this issue to block for weeks etc.:

adamkewley commented 1 year ago

@tgeijten the feature is now shipped in main, so you can try it out via a CI build and get back.

CI builds are here:

If I don't hear back in a few days, or a week, I'll ship it as-is!

tgeijten commented 1 year ago

It works, thank you! However, when you run a simulation it reverts back to the default visualization settings.

adamkewley commented 1 year ago

It works, thank you! However, when you run a simulation it reverts back to the default visualization settings.

Is that the case with one of the more recent commits (say, after 6e7a76f)?

It was behavior I noticed but I thought it was patched out

(the more recent commits are failing builds, but the failures are because of very strict flags on Ubuntu etc - the Windows builds should be ok)

tgeijten commented 1 year ago

Yes, it's fixed in later releases. I previously indeed only installed the most recent successful build 😄

adamkewley commented 1 year ago

It's my bad - I'm colorblind to the red vs. green a bit because I'm running Ubuntu/MacOS with -pedantic etc. and I usually only sort everything out later in a dev cycle

Thanks for testing this and getting back @tgeijten - I'll try and ship a release containing it later today or early next week!

adamkewley commented 1 year ago

Closing this for now - the later tweaks have been broken out into #787, #788, #789, and #790, which can be handled separately, rather than blocking shipping this faster