diff --git a/application/CMakeLists.txt b/application/CMakeLists.txt index 2a9f85e36a..1a27a0ad48 100644 --- a/application/CMakeLists.txt +++ b/application/CMakeLists.txt @@ -191,7 +191,7 @@ f3d_generate_cli_options( F3D_SOURCE_DIR "${F3D_SOURCE_DIR}" F3D_BINARY_DIR "${F3D_BINARY_DIR}" ENABLED_CONDITIONALS ${f3d_enabled_conditionals} # Do NOT quote so this stays a list - CPP_CLI_OPTIONS_TO_SKIP input;define;reset # Do NOT quote so this stays a list + CPP_CLI_OPTIONS_TO_SKIP input # Do NOT quote so this stays a list ) set(F3D_SOURCE_FILES diff --git a/application/F3DOptionsTools.cxx b/application/F3DOptionsTools.cxx index 039ec3e64b..d58042777f 100644 --- a/application/F3DOptionsTools.cxx +++ b/application/F3DOptionsTools.cxx @@ -26,11 +26,10 @@ namespace { /** * True boolean options need to be filtered out in ParseCLIOptions - * Also filter out special options like `define` and `reset` * This is the easiest, compile time way to do it */ constexpr std::array CLIBooleans = { "version", "help", "list-readers", "scan-plugins", - "list-rendering-backends", "define", "reset" }; + "list-rendering-backends" }; //---------------------------------------------------------------------------- /** @@ -275,12 +274,6 @@ F3DOptionsTools::OptionsDict F3DOptionsTools::ParseCLIOptions( std::vector> cxxoptsValues; auto cxxoptsInputPositionals = cxxopts::value>(positionals); - std::vector defines; - auto cxxoptsDefines = cxxopts::value>(defines); - - std::vector resets; - auto cxxoptsResets = cxxopts::value>(resets); - try { cxxopts::Options cxxOptions(execName, F3D::AppTitle); @@ -294,8 +287,6 @@ F3DOptionsTools::OptionsDict F3DOptionsTools::ParseCLIOptions( if (std::string(optionGroup.GroupName) == "Applicative") { group("input", "Input files", cxxoptsInputPositionals, ""); - group("D,define", "Define libf3d options", cxxoptsDefines, "libf3d.option=value"); - group("R,reset", "Reset libf3d options", cxxoptsResets, "libf3d.option"); } // Add each option to cxxopts @@ -313,12 +304,15 @@ F3DOptionsTools::OptionsDict F3DOptionsTools::ParseCLIOptions( std::string defaultValue; std::string helpText(cliOption.HelpText); std::string longName(cliOption.LongName); + std::string valueHelper(cliOption.ValueHelper); // Recover default value from app options auto appIter = F3DOptionsTools::DefaultAppOptions.find(longName); if (appIter != F3DOptionsTools::DefaultAppOptions.end()) { - defaultValue = appIter->second; + // defaults are always stored as strings + assert(std::holds_alternative(appIter->second)); + defaultValue = std::get(appIter->second); } else { @@ -351,7 +345,14 @@ F3DOptionsTools::OptionsDict F3DOptionsTools::ParseCLIOptions( } // Recover the implicit value and set it if any - cxxoptsValues.emplace_back(cxxopts::value()); + if (valueHelper == "") + { + cxxoptsValues.emplace_back(cxxopts::value>()); + } + else + { + cxxoptsValues.emplace_back(cxxopts::value()); + } auto& val = cxxoptsValues.back(); if (!cliOption.ImplicitValue.empty()) { @@ -438,33 +439,34 @@ F3DOptionsTools::OptionsDict F3DOptionsTools::ParseCLIOptions( throw F3DExFailure("unknown options"); } - // Add each CLI options into a vector of string/string and return it + // Add each CLI options into a OptionsDict and return it F3DOptionsTools::OptionsDict cliOptionsDict; for (const auto& res : result) { // Discard boolean option like `--version` or `--help` - if (std::ranges::find(::CLIBooleans, res.key()) == ::CLIBooleans.end()) + if (std::ranges::find(::CLIBooleans, res.key()) != ::CLIBooleans.end()) + { + continue; + } + auto iter = cliOptionsDict.find(res.key()); + if (iter == cliOptionsDict.end()) { cliOptionsDict[res.key()] = res.value(); } - } - - // Handle defines and add them as proper options - for (const std::string& define : defines) - { - std::string::size_type sepIdx = define.find_first_of('='); - if (sepIdx == std::string::npos) + else { - f3d::log::warn("Could not parse a define '", define, "'"); - continue; + // key already exists. Create or append to vector + if (std::holds_alternative>(iter->second)) + { + auto& vec = std::get>(iter->second); + vec.push_back(res.value()); + } + else + { + auto& item = std::get(iter->second); + cliOptionsDict[res.key()] = std::vector{ item, res.value() }; + } } - cliOptionsDict[define.substr(0, sepIdx)] = define.substr(sepIdx + 1); - } - - // Handles reset using the dedicated syntax - for (const std::string& reset : resets) - { - cliOptionsDict["reset-" + reset] = ""; } return cliOptionsDict; @@ -491,10 +493,11 @@ void F3DOptionsTools::PrintHelpPair( } //---------------------------------------------------------------------------- -std::vector> F3DOptionsTools::ConvertToLibf3dOptions( - const std::string& key, const std::string& value) +std::vector> +F3DOptionsTools::ConvertToLibf3dOptions(const std::string& key, const OptionValue& value) { - std::vector> libf3dOptions; + std::vector> libf3dOptions; + [[maybe_unused]] const bool is_single_valued = std::holds_alternative(value); // Simple one-to-one case auto libf3dIter = F3DOptionsTools::LibOptionsNames.find(key); @@ -506,20 +509,22 @@ std::vector> F3DOptionsTools::ConvertToLibf3 // anti-aliasing is handled in two options in the lib else if (key == "anti-aliasing") { - if (value != "none") + assert(is_single_valued); + const std::string& value_str = std::get(value); + if (value_str != "none") { // Handle deprecated boolean option bool deprecatedBooleanOption; - if (F3DOptionsTools::Parse(value, deprecatedBooleanOption)) + if (F3DOptionsTools::Parse(value_str, deprecatedBooleanOption)) { f3d::log::warn("--anti-aliasing is a now a string, please specify the type of " "anti-aliasing or use the implicit default"); - libf3dOptions.emplace_back(std::make_pair("render.effect.antialiasing.enable", value)); + libf3dOptions.emplace_back(std::make_pair("render.effect.antialiasing.enable", value_str)); } else { libf3dOptions.emplace_back(std::make_pair("render.effect.antialiasing.enable", "true")); - libf3dOptions.emplace_back(std::make_pair("render.effect.antialiasing.mode", value)); + libf3dOptions.emplace_back(std::make_pair("render.effect.antialiasing.mode", value_str)); } } else @@ -531,17 +536,21 @@ std::vector> F3DOptionsTools::ConvertToLibf3 // handle deprecated anti-aliasing option else if (key == "anti-aliasing-mode") { + assert(is_single_valued); + const std::string& value_str = std::get(value); f3d::log::warn("--anti-aliasing-mode is deprecated"); - libf3dOptions.emplace_back(std::make_pair("render.effect.antialiasing.mode", value)); + libf3dOptions.emplace_back(std::make_pair("render.effect.antialiasing.mode", value_str)); } // blending is handled in two options in the lib else if (key == "blending") { - if (value != "none") + assert(is_single_valued); + const std::string& value_str = std::get(value); + if (value_str != "none") { libf3dOptions.emplace_back(std::make_pair("render.effect.blending.enable", "true")); - libf3dOptions.emplace_back(std::make_pair("render.effect.blending.mode", value)); + libf3dOptions.emplace_back(std::make_pair("render.effect.blending.mode", value_str)); } else { @@ -552,14 +561,18 @@ std::vector> F3DOptionsTools::ConvertToLibf3 // handle deprecated translucency support else if (key == "translucency-support") { + assert(is_single_valued); + const std::string& value_str = std::get(value); f3d::log::warn("--translucency-support is deprecated, please use --blending instead"); - libf3dOptions.emplace_back(std::make_pair("render.effect.blending.enable", value)); + libf3dOptions.emplace_back(std::make_pair("render.effect.blending.enable", value_str)); } // point sprites is handled in two options in the lib else if (key == "point-sprites") { - if (value != "none") + assert(is_single_valued); + const std::string& value_str = std::get(value); + if (value_str != "none") { // Handle deprecated boolean option bool deprecatedBooleanOption; @@ -567,12 +580,12 @@ std::vector> F3DOptionsTools::ConvertToLibf3 { f3d::log::warn("--point-sprites is a now a string, please specify the type of " "point sprites to use or use the implicit default"); - libf3dOptions.emplace_back(std::make_pair("model.point_sprites.enable", value)); + libf3dOptions.emplace_back(std::make_pair("model.point_sprites.enable", value_str)); } else { libf3dOptions.emplace_back(std::make_pair("model.point_sprites.enable", "true")); - libf3dOptions.emplace_back(std::make_pair("model.point_sprites.type", value)); + libf3dOptions.emplace_back(std::make_pair("model.point_sprites.type", value_str)); } } else @@ -584,19 +597,22 @@ std::vector> F3DOptionsTools::ConvertToLibf3 // handle deprecated point-sprites-type option else if (key == "point-sprites-type") { + assert(is_single_valued); + const std::string& value_str = std::get(value); f3d::log::warn("--point-sprites-type is deprecated"); - libf3dOptions.emplace_back(std::make_pair("model.point_sprites.type", value)); + libf3dOptions.emplace_back(std::make_pair("model.point_sprites.type", value_str)); } // handle deprecated interaction-trackball option else if (key == "interaction-trackball") { + const std::string& value_str = std::get(value); f3d::log::warn( "--interaction-trackball is deprecated, please use --interaction-style=trackball instead"); bool trackball; - if (!F3DOptionsTools::Parse(value, trackball)) + if (!F3DOptionsTools::Parse(value_str, trackball)) { - f3d::log::error("Cannot parse --interaction-trackball value: " + value); + f3d::log::error("Cannot parse --interaction-trackball value: " + value_str); } else if (trackball) { @@ -612,3 +628,19 @@ std::vector> F3DOptionsTools::ConvertToLibf3 return libf3dOptions; } + +std::string F3DOptionsTools::ConvertToString(const F3DOptionsTools::OptionValue& optionValue) +{ + if (std::holds_alternative(optionValue)) + { + return std::get(optionValue); + } + else if (std::holds_alternative>(optionValue)) + { + const auto& vec = std::get>(optionValue); + auto concatenate = [](const std::string& result, const std::string& value) + { return result + " , " + value; }; + return std::accumulate(vec.cbegin(), vec.cend(), std::string{ "" }, concatenate); + } + return ""; +} diff --git a/application/F3DOptionsTools.h b/application/F3DOptionsTools.h index edb8746979..f272d9f2c9 100644 --- a/application/F3DOptionsTools.h +++ b/application/F3DOptionsTools.h @@ -9,13 +9,15 @@ #include #include +#include #include #include #include namespace F3DOptionsTools { -using OptionsDict = std::map; +using OptionValue = std::variant, std::string>; +using OptionsDict = std::map; using OptionsEntry = std::tuple; using OptionsEntries = std::vector; @@ -27,7 +29,7 @@ using OptionsEntries = std::vector; * and ParseCLIOptions */ static inline const OptionsDict DefaultAppOptions = { - { "input", "" }, + { "input", { "" } }, { "output", "" }, { "list-bindings", "false" }, { "no-background", "false" }, @@ -167,11 +169,16 @@ static inline const std::map CustomMappingOp { "point-sprites-type", "" }, }; +/** + * List of CLI option names that can accept multiple values + */ +static inline const std::set MultiValueOptions = { "define", "input", "reset" }; + /** * Convert a CLI options key/value to a vector of libf3d options key/value */ -std::vector> ConvertToLibf3dOptions( - const std::string& key, const std::string& value); +std::vector> ConvertToLibf3dOptions( + const std::string& key, const OptionValue& value); /** * Browse through all possible option names to find one that have the smallest distance to the @@ -196,13 +203,19 @@ F3DOptionsTools::OptionsDict ParseCLIOptions( void PrintHelpPair( std::string_view key, std::string_view help, int keyWidth = 10, int helpWidth = 70); +/** + * Convert the variant-based OptionValue to a string. Useful for debugging + */ +std::string ConvertToString(const OptionValue& optionValue); + /** * Parse provided string into provided typed var. * Return true if successful, false otherwise. */ template -bool Parse(const std::string& optionString, T& option) +bool Parse(const OptionValue& optionValue, T& option) { + const std::string optionString = ConvertToString(optionValue); try { option = f3d::options::parse(optionString); diff --git a/application/F3DStarter.cxx b/application/F3DStarter.cxx index bc65a16705..5b57eca021 100644 --- a/application/F3DStarter.cxx +++ b/application/F3DStarter.cxx @@ -71,12 +71,58 @@ f3d::interactor* GlobalInteractor = nullptr; constexpr std::string_view F3D_PIPED = "-"; +namespace +{ +// helper to apply Callable to all items of OptionValue no matter if it is a +// single valued string or a vector of strings +// based on example at https://en.cppreference.com/cpp/utility/variant/visit2 + +template +void apply_to_all(F3DOptionsTools::OptionValue& option, Callable f) +{ + std::visit( + [f](auto&& value) + { + using T = std::decay_t; + if constexpr (std::is_same_v) + { + f(value); + } + else if constexpr (std::is_same_v>) + { + for (auto& item : value) + { + f(item); + } + } + else + { + static_assert(false, "non-exhaustive visitor!"); + } + }, + option); +} +void HandleDefine(f3d::options& libOptions, const std::string& optionValue) +{ + std::string::size_type sepidx = optionValue.find_first_of('='); + if (sepidx == std::string::npos) + { + f3d::log::warn("Could not parse a define '", optionValue, "'"); + return; + } + const auto key = optionValue.substr(0, sepidx); + const auto value = optionValue.substr(sepidx + 1); + libOptions.setAsString(key, value); +} +} + class F3DStarter::F3DInternals { public: F3DInternals() = default; - using log_entry_t = std::tuple; + using log_entry_t = + std::tuple; // XXX: The values in the following two structs // are left uninitialized as the will all be initialized from @@ -570,7 +616,8 @@ class F3DStarter::F3DInternals { const auto& [bindStr, source, matchType, match, commands] = tuple; const std::string origin = F3DInternals::FormatOrigin(source, matchType, match); - f3d::log::debug(" '", bindStr, "' ", sep, " '", commands, "' from ", origin); + f3d::log::debug(" '", bindStr, "' ", sep, " '", F3DOptionsTools::ConvertToString(commands), + "' from ", origin); } f3d::log::debug(""); } @@ -643,45 +690,50 @@ class F3DStarter::F3DInternals { auto [libf3dOptionName, libf3dOptionValue] = libf3dOption; - bool reset = false; - - // Handle options reset - if (libf3dOptionName.starts_with("reset-")) - { - if (libf3dOptionName.size() > 6) - { - reset = true; - libf3dOptionName = libf3dOptionName.substr(6); - keyForLog = libf3dOptionName; - libf3dOptionValue = "reset"; - } - else - { - f3d::log::warn("Invalid option: 'reset' must be followed by a valid option " - "name, ignoring entry"); - continue; - } - } - // Handle reader options std::vector readerOptionNames = f3d::engine::getAllReaderOptionNames(); if (std::ranges::find(readerOptionNames, libf3dOptionName) != readerOptionNames.end()) { - f3d::engine::setReaderOption(libf3dOptionName, libf3dOptionValue); + assert(std::holds_alternative(libf3dOptionValue)); + std::string libf3dOptionValueStr = std::get(libf3dOptionValue); + + f3d::engine::setReaderOption(libf3dOptionName, libf3dOptionValueStr); continue; } try { - // Assume this is a libf3d option and set/reset the value - if (reset) + if (libf3dOptionName == "reset") + { + apply_to_all(libf3dOptionValue, + [&libOptions, &keyForLog](const std::string& optionValue) + { + keyForLog = optionValue; + if (optionValue.empty()) + { + f3d::log::warn("Invalid option: 'reset' must be followed by a valid " + "option name, ignoring entry"); + return; + } + libOptions.reset(optionValue); + }); + } + else if (libf3dOptionName == "define") { - libOptions.reset(libf3dOptionName); + apply_to_all(libf3dOptionValue, + [&libOptions, &keyForLog](const std::string& optionValue) + { + keyForLog = optionValue; + HandleDefine(libOptions, optionValue); + }); } else { - libOptions.setAsString(libf3dOptionName, libf3dOptionValue); + assert(std::holds_alternative(libf3dOptionValue)); + const std::string libf3dOptionValueStr = + std::get(libf3dOptionValue); + libOptions.setAsString(libf3dOptionName, libf3dOptionValueStr); } // Log the option if needed @@ -696,8 +748,9 @@ class F3DStarter::F3DInternals if (!quiet) { const std::string origin = F3DInternals::FormatOrigin(source, matchType, match); - f3d::log::warn("Could not set '", keyForLog, "' to '", libf3dOptionValue, - "' from ", origin, " because: ", ex.what()); + f3d::log::warn("Could not set '", keyForLog, "' to '", + F3DOptionsTools::ConvertToString(libf3dOptionValue), "' from ", origin, + " because: ", ex.what()); } } catch (const f3d::options::inexistent_exception&) @@ -705,8 +758,7 @@ class F3DStarter::F3DInternals if (!quiet) { const std::string origin = F3DInternals::FormatOrigin(source, matchType, match); - auto [closestName, dist] = - F3DOptionsTools::GetClosestOption(libf3dOptionName, true); + auto [closestName, dist] = F3DOptionsTools::GetClosestOption(keyForLog, true); f3d::log::warn("'", keyForLog, "' option from ", origin, " does not exists , did you mean '", closestName, "'?"); } @@ -749,7 +801,8 @@ class F3DStarter::F3DInternals { if (!F3DOptionsTools::Parse(appOptions.at(name), option)) { - f3d::log::warn("Could not parse '" + appOptions.at(name) + "' into '" + name + "' option"); + f3d::log::warn("Could not parse '" + F3DOptionsTools::ConvertToString(appOptions.at(name)) + + "' into '" + name + "' option"); } } @@ -761,7 +814,7 @@ class F3DStarter::F3DInternals void ParseOption(const F3DOptionsTools::OptionsDict& appOptions, const std::string& name, std::optional& option) { - const std::string& optStr = appOptions.at(name); + const std::string& optStr = F3DOptionsTools::ConvertToString(appOptions.at(name)); if (optStr.empty()) { option = std::nullopt; @@ -1069,8 +1122,8 @@ int F3DStarter::Start(int argc, char** argv) { if (!F3DOptionsTools::Parse(iter->second, noConfig)) { - f3d::log::warn( - "Could not parse '" + iter->second + "' into 'no-config' option, assuming false"); + f3d::log::warn("Could not parse '" + F3DOptionsTools::ConvertToString(iter->second) + + "' into 'no-config' option, assuming false"); } } @@ -1568,6 +1621,7 @@ void F3DStarter::LoadFileGroupInternal( // options names are shared between options instance F3DOptionsTools::OptionsDict dynamicOptionsDict; std::vector optionNames = dynamicOptions.getAllNames(); + std::vector resets; for (const auto& name : optionNames) { if (!dynamicOptions.isSame(this->Internals->LibOptions, name)) @@ -1576,7 +1630,7 @@ void F3DStarter::LoadFileGroupInternal( { // If a dynamic option has been changed and does not have value, it means it was reset using // the command line reset it using the dedicated syntax - dynamicOptionsDict["reset-" + name] = ""; + resets.push_back(name); } else { @@ -1586,6 +1640,7 @@ void F3DStarter::LoadFileGroupInternal( } } } + dynamicOptionsDict["reset"] = resets; // Detect interactively changed verbose level and add it to dynamic options f3d::log::VerboseLevel currentVerboseLevel = f3d::log::getVerboseLevel(); diff --git a/application/testing/tests.features.cmake b/application/testing/tests.features.cmake index 4a8b00ae74..0cdb462fbb 100644 --- a/application/testing/tests.features.cmake +++ b/application/testing/tests.features.cmake @@ -232,13 +232,16 @@ f3d_test(NAME TestFontColor DATA suzanne.ply ARGS -n --font-color=Orange UI) ## Special CLI syntax f3d_test(NAME TestDefines DATA dragon.vtu ARGS -Dscene.up_direction=+Z --define=model.color.rgb=red) +f3d_test(NAME TestDefinesMultiple DATA dragon.vtu ARGS --define=scene.up_direction=+Z --define=model.color.rgb=red) f3d_test(NAME TestDefinesInvalid DATA dragon.vtu ARGS -Dscene.up_direction+Z REGEXP "Could not parse a define" NO_BASELINE) f3d_test(NAME TestDefinesInexistent DATA dragon.vtu ARGS -Dscene.up_director=+Z REGEXP "option from CLI options does not exists" NO_BASELINE) f3d_test(NAME TestAlternativeOptionSyntax DATA WaterBottle.glb ARGS --max-size 0.2 REGEXP "file is bigger than max size" NO_BASELINE) f3d_test(NAME TestCustomOptionsNone DATA red_translucent_monkey.gltf ARGS --blending=none --anti-aliasing=none --point-sprites=none) +f3d_test(NAME TestDefineAndReset DATA dragon.vtu ARGS -Drender.show_edges=1 --define=render.background.color=1,1,1 -Rrender.show_edges --reset=render.background.color) ## Config f3d_test(NAME TestConfigReset DATA suzanne.stl ARGS -Rrender.grid.enable --reset=ui.axis CONFIG ${F3D_SOURCE_DIR}/testing/configs/complex.json) +f3d_test(NAME TestConfigResetMultiple DATA suzanne.stl ARGS --reset=render.grid.enable --reset=ui.axis CONFIG ${F3D_SOURCE_DIR}/testing/configs/complex.json) f3d_test(NAME TestConfigResetInexistent DATA suzanne.stl ARGS -Rrender.glid.enable REGEXP "option from CLI options does not exists" NO_BASELINE) f3d_test(NAME TestConfigOrder DATA suzanne.ply ARGS CONFIG ${F3D_SOURCE_DIR}/testing/configs/config_order.json) # `.+` > `.*` alphabetically but overridden by the order # Needs https://gitlab.kitware.com/vtk/vtk/-/merge_requests/12489 diff --git a/resources/cli-options.json b/resources/cli-options.json index caac810241..6e2ac2a179 100644 --- a/resources/cli-options.json +++ b/resources/cli-options.json @@ -12,12 +12,14 @@ { "longName": "define", "shortName": "D", - "helpText": "Define libf3d options" + "helpText": "Define libf3d options", + "valueHelper": "" }, { "longName": "reset", "shortName": "R", - "helpText": "Reset libf3d options" + "helpText": "Reset libf3d options", + "valueHelper": "" }, { "longName": "output", diff --git a/testing/baselines/TestConfigResetMultiple.png b/testing/baselines/TestConfigResetMultiple.png new file mode 100644 index 0000000000..ceb11bd00f --- /dev/null +++ b/testing/baselines/TestConfigResetMultiple.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:2e6e2b95e9ad761851cc0c767bcc4c04d0e9a38687350bf5401fd4f4da630789 +size 23695 diff --git a/testing/baselines/TestDefineAndReset.png b/testing/baselines/TestDefineAndReset.png new file mode 100644 index 0000000000..70da184380 --- /dev/null +++ b/testing/baselines/TestDefineAndReset.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:64feff09fbb315240054a0c163dbd97e18f00ab8732cb1f9719bc39496baacf5 +size 22986 diff --git a/testing/baselines/TestDefinesMultiple.png b/testing/baselines/TestDefinesMultiple.png new file mode 100644 index 0000000000..ebb3f7d0f2 --- /dev/null +++ b/testing/baselines/TestDefinesMultiple.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:ed55b928b8bfd5d027840d84487eaaca5945a5d8fcacc3b792204f92b0529f53 +size 26830