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

Rework applicative options #1580

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

mwestphal
Copy link
Contributor

@mwestphal mwestphal commented Aug 23, 2024

Complete refactoring of the options parsing mechanism in F3D application.

Implementation:
- Create a new F3DOptionsTools namespace that declare all app options and corresponding libf3d options and parse CLI
- Expose F3DConfigFileTools and update the config parsing feature
- Rewrite fully F3DStarter to use F3DOptionsTools and F3DConfigFileTools relying on a clear logic

The logic is:
- Parse the CLI once, store the CLI options in a single OptionsDict in a OptionsEntries
- Parse the config file once, store in a OptionEntries
- Update the app and lib options with config < cli logic without a provided filename
- Finish engine, window initialization
- Create a dynamic OptionsDict containing the changed options and emplace in DynamicOptionsEntries
- Update the app and lib options with config < cli < dynamic logic with filename

Other features:
- --input is back as a proper positional cli option!
- --scalars, -s is now split into --scalar-coloring, -s and --coloring-array, with F3D, F3DWeb, libf3d and vtkext related changes
- Add support for using libf3d option names in config
- Add GetClosestOption for configs
- Remove support for f3d --option val, = must be used: jarro2783/cxxopts#211

Fixes: #1569 #434 #1114

In other PRs:

@mwestphal mwestphal marked this pull request as draft August 23, 2024 05:17
@mwestphal mwestphal force-pushed the rework_applicative_options branch 2 times, most recently from dd2fd46 to 1fc20c5 Compare August 23, 2024 18:41
application/F3DStarter.cxx Outdated Show resolved Hide resolved
@mwestphal mwestphal marked this pull request as ready for review August 25, 2024 06:44
@mwestphal mwestphal force-pushed the rework_applicative_options branch from ae494db to 59c1dc6 Compare August 28, 2024 05:25
@mwestphal mwestphal requested a review from Meakk August 28, 2024 06:13
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 97.72257% with 11 lines in your changes missing coverage. Please review.

Project coverage is 96.82%. Comparing base (6ef173d) to head (0555a6e).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
application/F3DSystemTools.cxx 77.27% 5 Missing ⚠️
application/F3DConfigFileTools.cxx 94.33% 3 Missing ⚠️
application/F3DOptionsTools.cxx 98.94% 2 Missing ⚠️
application/F3DStarter.cxx 99.46% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1580      +/-   ##
==========================================
- Coverage   96.85%   96.82%   -0.04%     
==========================================
  Files         107      108       +1     
  Lines        7694     7643      -51     
==========================================
- Hits         7452     7400      -52     
- Misses        242      243       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mwestphal mwestphal force-pushed the rework_applicative_options branch 4 times, most recently from db0195d to 079b64b Compare August 30, 2024 09:12
@snoyer
Copy link
Contributor

snoyer commented Aug 30, 2024

There has been an issue in the past with cxxopts being too eager to split arguments into vectors; #1078 fixed it with a workaround because cxxopts's splitting was needed for point3/vector3/color arguments.
However now that f3d parses vectors itself if would be worth checking if we can disable cxxopts splitting entirely and possibly remove the workaround.

#define CXXOPTS_VECTOR_DELIMITER '\0' would tell cxxopts to split on null and therefore never split.

application/F3DCLIOptionsTools.cxx Outdated Show resolved Hide resolved
application/F3DCLIOptionsTools.cxx Outdated Show resolved Hide resolved
application/F3DCLIOptionsTools.h Outdated Show resolved Hide resolved
vtkext/private/module/vtkF3DConfigure.h.in Outdated Show resolved Hide resolved
@mwestphal
Copy link
Contributor Author

There has been an issue in the past with cxxopts being too eager to split arguments into vectors; #1078 fixed it with a workaround because cxxopts's splitting was needed for point3/vector3/color arguments. However now that f3d parses vectors itself if would be worth checking if we can disable cxxopts splitting entirely and possibly remove the workaround.

#define CXXOPTS_VECTOR_DELIMITER '\0' would tell cxxopts to split on null and therefore never split.

Absolutely correct! --input is back!

@mwestphal mwestphal force-pushed the rework_applicative_options branch 2 times, most recently from af4af35 to 0bc1752 Compare August 31, 2024 10:25
@mwestphal mwestphal requested a review from Meakk August 31, 2024 12:29
@mwestphal mwestphal force-pushed the rework_applicative_options branch 8 times, most recently from d5a184e to 9d0ee0d Compare September 1, 2024 12:07
@mwestphal mwestphal force-pushed the rework_applicative_options branch 2 times, most recently from abb270f to 934cdb0 Compare September 1, 2024 12:32
@mwestphal mwestphal changed the title [draft] Rework applicative options Rework applicative options Sep 1, 2024
application/F3DConfigFileTools.cxx Outdated Show resolved Hide resolved
application/F3DOptionsTools.cxx Outdated Show resolved Hide resolved
application/F3DOptionsTools.cxx Outdated Show resolved Hide resolved
application/F3DOptionsTools.cxx Outdated Show resolved Hide resolved
application/F3DOptionsTools.cxx Outdated Show resolved Hide resolved
@mwestphal mwestphal force-pushed the rework_applicative_options branch from 934cdb0 to 86ebfbd Compare September 1, 2024 15:21
@mwestphal mwestphal requested a review from Meakk September 1, 2024 15:24
Copy link
Member

@Meakk Meakk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last minor nitpicking but please change the web app too.

application/F3DConfigFileTools.cxx Outdated Show resolved Hide resolved
application/F3DOptionsTools.cxx Show resolved Hide resolved
application/F3DOptionsTools.cxx Outdated Show resolved Hide resolved
application/F3DOptionsTools.cxx Outdated Show resolved Hide resolved
application/F3DStarter.cxx Outdated Show resolved Hide resolved
application/F3DStarter.cxx Outdated Show resolved Hide resolved
@mwestphal mwestphal force-pushed the rework_applicative_options branch from 86ebfbd to bac40fd Compare September 2, 2024 15:46
Implementation:
- Create a new F3DOptionsTools namespace that declare all app options and corresponding libf3d options and parse CLI
- Expose F3DConfigFileTools and update the config parsing feature
- Rewrite fully F3DStarter to use F3DOptionsTools and F3DConfigFileTools relying on a clear logic

The logic is:
 - Parse the CLI once, store the CLI options in a single OptionsDict in a OptionsEntries
 - Parse the config file once, store in a OptionEntries
 - Update the app and lib options with `config < cli` logic without a provided filename
 - Finish engine, window initialization
 - Create a dynamic OptionsDict containing the changed options and emplace in DynamicOptionsEntries
 - Update the app and lib options with `config < cli < dynamic ` logic with filename

 Other features:
 - `--input` is back as a proper positional cli option!
 - Add support for using libf3d option names in config
 - Add GetClosestOption for configs
…--coloring-array`

 - Remove `--scalars, -s`
 - Introduce `--scalar-coloring, -s` option and corresponding libf3d option
 - Introduce `coloring-array` option and corresponding libf3d option
 - Update tests and configs
 - Add an `enable` boolean to the SetColoring API
 - Update API usage in window and interactor
 - Remove fully F3D_RESERVED* constants
@mwestphal mwestphal force-pushed the rework_applicative_options branch from bac40fd to 0555a6e Compare September 2, 2024 15:47
@mwestphal mwestphal requested a review from Meakk September 2, 2024 16:02
@mwestphal mwestphal merged commit 9ee6912 into f3d-app:master Sep 2, 2024
39 checks passed
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

Successfully merging this pull request may close these issues.

Rework application layer and simplify options logic
3 participants