[Obsolete] A new composition parser for all features#926
Conversation
alanjyu
force-pushed
the
composition_name_parse
branch
from
2be23d5 to
e468287
Compare
danieldouglas92
left a comment
danieldouglas92
left a comment
There was a problem hiding this comment.
Hey @alanjyu thanks for this! If I'm understanding correctly, a lot of these changes here are just renaming what you added in PR #909, and then the new get_vector() function you added in parameters.cc is the new feature here? I didn't get a chance to look at #909, but I think being able to set compositions in the world builder file with a string will be a very nice quality of life improvement. I've forgotten so many times what index number N in some of my large world builder files is actually supposed to correspond to.
Anyway, if what I said above is correct and it's not too much of a hassle, I would just open up a very quick PR that only renames Parameters::composition_properties to Parameters::composition_property.
I think the new parser function looks good, I only had a clarification comment about the modification you made to the existing get_vector parser.
There was a problem hiding this comment.
You'll have to delete this file (hopefully now that #927 has been merged it will never be an issue🤞)
| template<> | ||
| std::vector<unsigned int> | ||
| Parameters::get_vector(const std::string &name, | ||
| const std::vector<Parameters::composition_property> &composition_properties) | ||
| { | ||
| std::vector<unsigned int> vector; | ||
| const std::string strict_base = this->get_full_json_path(); | ||
| if (Pointer((strict_base + "/" + name).c_str()).Get(parameters) != nullptr) | ||
| { | ||
| Value *array = Pointer((strict_base + "/" + name).c_str()).Get(parameters); | ||
|
|
||
| for (size_t i = 0; i < array->Size(); ++i ) | ||
| { | ||
| const std::string base = (strict_base + "/").append(name).append("/").append(std::to_string(i)); | ||
| Value *entry = Pointer(base.c_str()).Get(parameters); | ||
|
|
||
| // user can define either an index (usigned int) | ||
| // or a compositon name (string) | ||
| // if latter, assign the corresponding index in the composition properties | ||
| if (entry->IsUint()) | ||
| { | ||
| vector.push_back(entry->GetUint()); | ||
| } | ||
| else if (entry->IsString()) | ||
| { | ||
| const std::string feature_composition_name = entry->GetString(); | ||
| bool isFoundInCompositionProperties = false; | ||
|
|
||
| for (const Parameters::composition_property &global_composition : composition_properties) | ||
| { | ||
| // compare globally defined composition name | ||
| // with feature-defined composition name | ||
| // and assign the corresponding index if found | ||
| if (global_composition.name == feature_composition_name) | ||
| { | ||
| vector.push_back(global_composition.index); | ||
| isFoundInCompositionProperties = true; | ||
| break; | ||
| } | ||
| } | ||
| WBAssertThrow(isFoundInCompositionProperties, | ||
| "internal error: could not find the value \"" << feature_composition_name << "\" in the composition properties at: " | ||
| << this->get_full_json_schema_path() + "/" + name + "/items/enum"); | ||
| } | ||
| else | ||
| { | ||
| WBAssertThrow(false, | ||
| "internal error: expected an unsigned int or a string for the value at: " | ||
| << base); | ||
| } |
There was a problem hiding this comment.
This function that you've added is how you handle the case for the composition being entered as a string instead of an integer. But I'm not sure what you added to the other get_vector function above. Can you add some documentation in that function that describes what the else statement is meant to handle in the function above?
|
|
Please let us know when it is ready for another round of reviews. Also, I think it would be good to rebase onto the newest main and regenerated the declarations (by running make test). |
alanjyu
changed the title
|
Changes are re-applied in a new pull request #934. |
3 participants
WARNING: "Parameter::composition_properties" (struct) has been renamed to "composition_property" (storing index, name, density, etc. for one composition), while the global composition_properties (vector) indicates the user defined table. These two were named the same in the previous push request.
Users now can assign a composition field using a name (string) that they define in the composition_properties table. The name will be parsed and linked to the corresponding composition index (and default properties). The name output function (what you see in ParaView) has not been updated yet.