--Add ConfigValue support for Mn::Matrix3 (3x3 matrices of floats) #1753
Conversation
facebook-github-bot
added
the
CLA Signed
src/esp/core/Configuration.cpp
Outdated
| Magnum::Quaternion val{}; | ||
| if (io::fromJsonValue(obj, val)) { | ||
| set(key, val); | ||
| } else if (obj[0].IsArray() && obj[0][0].IsNumber()) { |
There was a problem hiding this comment.
I suppose this writes a JSON in a form of
[[1, 0, 0],
[0, 1, 0],
[0, 0, 1]] right? I'd prefer an one-dimensional array instead, because:
- this is how glTF does it (in case you'd eventually want JSON compatibility to some degree)
- it's faster to parse, faster to read, faster to write, faster to validate (one just dumps array of 9 values, or checks that the numeric array has 9 values; which is especially convenient with the new Utility::JsonToken::asFloatArray() and Utility::JsonWriter::writeArray() APIs, and the also new Matrix::data() API that returns a sized array, thus the copy from/to JSON can be a single
Utility::copy(json.asFloatArray(), matrix.data())/json.writeFloatArray(matrix.data())call) - there doesn't need to be any extra logic to flip from a column-major representation to a row-major representation, it's just a data copy
There was a problem hiding this comment.
My original thinking was that I wanted to make something that was a bit more human-readable, but I did notice the gltf standard (Which I am shooting for strict adherence to) so doing things this way makes sense to me.
| } | ||
| Cr::Utility::formatInto(res, res.length(), "]"); | ||
| return res; | ||
| } |
There was a problem hiding this comment.
Ah wait, why is this class reinventing Magnum's own builtin debug output for the vectors, quaternions and radians? It's as simple as this, for any Magnum type:
std::ostringstream out;
Mn::Debug{&out, Mn::Debug::Flag::NoNewlineAtTheEnd} << get<T>();
return out.str();Besides minor formatting differences, I don't see why you would have to write all that code again.
There was a problem hiding this comment.
The to-strings were done this way originally (with vec3 and quats) to remove the commas between the elements (this function was being used to provide data in a csv-friendly format). I figured that if folks wanted the magnum-based stream version, they could get it easily enough in the manner you described.
There was a problem hiding this comment.
@mosra Is there a flag for Magnum::Debug::Flag for NoComma?
There was a problem hiding this comment.
I was looking for that, but it looks like the stream functions specify the comma explicitly
There was a problem hiding this comment.
Yeah the formatting is static because the intention is for it to be human-readable, not machine-consumable. Though in your case it's not CSV-friendly either due to the [ ]... :)
I have a different idea for a generic machine-readable output -- all Math classes have a .data() accessor, which (in latest Magnum) provides a sized array reference. So then you could have a single function, such as printCsvFriendly(Cr::Containers::ArrayView<const float>), which converts a numeric array into a space-delimited string, and then use it for any Math type: printCsvFriendly(matrix.data()) etc.
tests/test_configs.py
Outdated
| # Magnum::Matrix3 (3x3) | ||
| my_mat3x3 = mn.Matrix3((1.1, 2.2, -3.3), (4.4, 5.0, -6.6), (7.7, 8.0, -9.9)) | ||
| config.set("mat3x3", my_mat3x3) | ||
| assert config.get("mat3x3") |
There was a problem hiding this comment.
| assert config.get("mat3x3") | |
| assert config.get("mat3x3") == my_mat3x3 |
? 👀
Motivation and Context
This PR adds support for 3x3 float Magnum Matrices to the ConfigValue/Configuration subsystem. This includes writing/reading json values, as well internally storing the data within a ConfigValue structure.
IOTest has also been streamlined somewhat, to make adding tests to validate json read/write for new types easier.
How Has This Been Tested
Locally c++ and python tests pass. CoreTest and IOTest have been modified to test appropriate functionality.
Types of changes
Checklist