-
Notifications
You must be signed in to change notification settings - Fork 400
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
esp::core::Configuration changed to use unordered_map of variant-like instead of Magnum::ConfigurationGroups #1433
Conversation
258f837
to
52ed767
Compare
Really would like to hear @mosra's input on this. Might be a better solution. |
484205c
to
144b87d
Compare
Discussing this refactor with @mosra, he suggested I investigate a tagged union storage container, so this push introduces this. Now all variables are stored in a single map, regardless of type, and accessed appropriately based on the "tag"-specified type they are. A std::variant/std::visitor system would have been safer and more streamlined, but these require c++17, which we do not currently support. |
41ba02c
to
813ccca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall seems good. It is quite verbose, e.g. lots of switch statements... hopefully we aren't adding new types often.
I spotted a number of changes that seemed to be unrelated to the config stuff. Let's move them to separate PRs so they get properly reviewed.
Side note: we should move to C++17 soon! I guess a lot of this is re-inventing std::any/std::variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like Mosra's opinion before merge. Overall, I like this approach.
I chatted with @jturner65 and came up with a couple of other nice-to-have features:
- a query for all keys as a map/dict (key -> type enum) and supporting type enum bindings.
- a get(key, type) python function for easy access out of the dict returned by (1.)
- sub configuration accessor bindings: get a sub configuration as a separate Configuration object (nested hierarchal structure) with option to get copy or ref and to register modified copies back into a subconfiguration
- query list of sub configuration keys
229317a
to
c028559
Compare
return std::to_string(d); | ||
case ConfigStoredType::String: | ||
return s; | ||
case ConfigStoredType::MagnumVec3: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't Magnum have some sort of serializer for these types @mosra?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has, one for debug output (where the goal is human readability) and one for (INI-style, space-delimited) configuration values. Neither of those produces what's supposedly needed here, tho (JSON-like formatting).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the python bindings and the "other code" first, will focus on the internals in a separate review.
const Magnum::Quaternion& val) { | ||
const auto ptr = self.editUserConfiguration(); | ||
return ptr->set(key, val); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those could all be just set_user_config
overloads without the type name, no? Or, even more pythonic, implement either a __setitem__
operator or __setattr__
, so instead of
foo.set_user_config("bar", bar)
you'd do
foo.user_config['bar'] = bar
or
foo.user_config.bar = bar
The last one might be a bit controversial, argparse
uses it for example but it might be confusing to some. Ask the others for opinion first :)
src/esp/bindings/CoreBindings.cpp
Outdated
py::enum_<ConfigStoredType>(m, "ConfigStoredType") | ||
.value("Unknown", ConfigStoredType::Unknown) | ||
.value("Boolean", ConfigStoredType::Boolean) | ||
.value("Integer", ConfigStoredType::Integer) | ||
.value("Double", ConfigStoredType::Double) | ||
.value("String", ConfigStoredType::String) | ||
.value("MagnumVec3", ConfigStoredType::MagnumVec3) | ||
.value("MagnumQuat", ConfigStoredType::MagnumQuat) | ||
.value("MagnumRad", ConfigStoredType::MagnumRad); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion is out of my league and I don't know how to express it via pybind (maybe @Skylion007 could help?) but what about get_type()
returning a Python type object? So you'd have e.g.
foo.set_string('bar', 'a string')
assert foo.get_type('bar') == str
Same comment about API naming here as well, could be just foo.bar = 'a string'
. And because I think some minor overhead doesn't matter that much, getting the type could be as simple as this, instead of foo.get_type('bar')
-- because with the interface designed like this, I think the need for querying just a type and not the value is very minimal:
type(foo.bar)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py::type::of
is what you are looking for: pybind/pybind11#2364
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do need to have an equivalent to 'ConfigStoredType::unknown' to handle the edge case where the value has not been properly initialized. Is there a python type equivalent i could use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py::none
would be the thing, I'd say? Similar semantics to a null pointer.
src/esp/bindings/CoreBindings.cpp
Outdated
.def( | ||
"has_rad", | ||
[](Configuration& self, const std::string& key) { | ||
return self.checkMapForKeyAndType(key, ConfigStoredType::MagnumRad); | ||
}, | ||
R"(Returns true if specified key references a Magnum::Rad value in this configuration.)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about usefulness of the has_*
APIs, now that you have everything stored in a single map there isn't a possibility of the same key being used for two different types and so this could be expressed simply as this and there's no need to have a dedicated overload for each and every type.
foo.get_type('bar') == mn.Rad
(assuming the above suggestion with get_type()
returning a Python type object, which makes it a lot shorter than ConfigStoredType.MagnumRad
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to strip out all the string concatenation changes and put them in another PR. (The only modifications will be what are required to support the underlying changes in the Configuration).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another batch, about the internals, I still need to look deeper into the breadcrumb parts.
Sorry in advance for all the comments! :)
|
Just FYI, when I said safer and more streamlined, I meant as compared to the former tagged-union refactor that this PR originally introduced. The system that I replaced it with (with substantial guidance from @mosra ) does not have the same safety concerns. |
Re On the other hand, About python bindings, I think we're fine here, exposing all types in a Pythonic way wasn't too complicated. |
6702022
to
9d11bef
Compare
6025609
to
8ba4ed8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Once the last couple review changes are addressed I think we can ship this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more bits to add to #1452, it was easier to add the review here because this PR has the full code.
R"(Retrieves a string representation of the value referred to by the passed key.)") | ||
|
||
.def("get_bool_keys", | ||
&Configuration::getStoredKeys<ConfigStoredType::Boolean>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last thing, I don't feel getStoredKeys()
needs to be templated, instead it could take the type as a parameter (getKeysByType()
and since you have the ConfigStoredType
exposed anyway, all these overloads could be just one get_keys_by_type()
with the ConfigStoredType
as a parameter.
"key"_a) | ||
|
||
.def( | ||
"has_bool", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, there could be a single hasKeyOfType()
/ has_key_of_type()
, taking a ConfigStoredType
.
return std::to_string(r.operator float()); | ||
} | ||
default: | ||
ESP_CHECK(true, "Unknown/unsupported Type in ConfigValue::getAsString."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is always true, and the error message will never ever be displayed.
Is it a bug here?
consider using:
CORRADE_INTERNAL_ASSERT_UNREACHABLE();
return ""; // dummy to avoid compile warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of similar places like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These checks are reachable only if a new type was added to the ConfigStoredType enumeration but not supported by case entries. The ESP_CHECKs were added to provide feedback to illuminate this condition so that it may be remedied, should other safeguards against this situation fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, missed that, should have been false
. @jturner65 can you fix these? Basically all occurences of ESP_CHECK(true, ...
are wrong. And since it's unlikely to be hit from user code, even less Python code, it doesn't need to be an ESP_CHECK()
either. This is what you're looking for:
CORRADE_ASSERT_UNREACHABLE("Unknown/unsupported Type in ConfigValue::getAsString");
(btw., @bigbike, the return
isn't needed, the macro contains a compiler-specific "unreachable code" annotation which does the right thing with no warnins)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case ConfigStoredType::MagnumRad: | ||
return cfg.setValue(key, get<Mn::Rad>()); | ||
default: | ||
ESP_CHECK( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is another place that needs to be fixed.
Motivation and Context
Currently Habitat-sim uses Magnum::ConfigurationGroups as the backing datastructure for all metadata loaded via JSON configuration files. A ConfigurationGroup saves all data as strings, and the original type of that data is lost once it is loaded into the system. This is particularly problematic with the user-defined attributes that habitat now supports. Upon read, the type of the user-defined value is inferred by the format of the data in the source JSON. With the upcoming functionality to save JSON configs to disk, including user_defined values, without having the type known will cause all data to be saved to disk as strings.
This PR replaces the ConfigurationGroup as the container for esp::core::Configuration and instead uses a ( collection of typed std::unordered_maps. ) single unordered map of ConfigValues, which use a tagged union/variant structure to retain type information and safety.
One question to consider : do we wish to remove the ConfigurationGroup as the backing structure for esp::core::Configuration. A different structure can be easily designed to replace esp::core::Configuration as the base class for the various configuration attributes that Habitat uses.
If nobody is using an esp::core::Configuration currently outside of the Metadata attributes/configuration subsystem, then the changes proposed by this PR can probably proceed with impunity; if, however, the ConfigurationGroup functionality backing habitat's Configuration is desired to remain, then I should instead create another class to back the attributes.
How Has This Been Tested
Locally, all c++ and python tests pass.
Types of changes
Checklist