-
Notifications
You must be signed in to change notification settings - Fork 361
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
CR167: GLTF Import - Metadata support #184
Conversation
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.
Dear dkulikov, here are few remarks,
Additionally, any changes please combine into single.
And please update the commit first line to have #184 directly at the end. You can check any other newest comments.
@@ -297,17 +297,19 @@ protected: | |||
void bindNodeShape (TopoDS_Shape& theShape, | |||
const TopLoc_Location& theLoc, | |||
const TCollection_AsciiString& theNodeId, | |||
const RWGltf_JsonValue* theUserName) | |||
const RWGltf_JsonValue* theUserName, |
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 would suggest to use const Handle(TDataStd_NamedData)&
.
Originally input parameter must be not updated. It is originally a part of the new internal class.
The same for 2 more new methods in current file
|
||
//! Parses the "extras" value provided in the constructor. | ||
//! @return Container with parsed data. May be nullptr if failed to parse. | ||
Handle(TDataStd_NamedData) Parse(); |
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.
But the myResult allocated always. It is not related?
Can be it null? Or better to avoid allocate ahead?
tests/metadata/gltf/end
Outdated
# Compare metadata in files. | ||
set aMetaDataLines [split ${aMetaData} "\n"] | ||
set aRefMetaDataLines [split ${ref_metadata} "\n"] | ||
set aMetaDataLinesCount [llength $aMetaDataLines] |
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.
Please clear up the short tabs from here.
Imposters here :)
Limited support of metadata import is added to RWGltf_GltfJsonParser. Following Json data types are currently supported: int32, double, string, array of int32, array of double, array of strings, Json object. Notable unsupported types are: binary data, array of Json objects. Metadata is processed for nodes and meshes. Tests "gltf_export" are updated with import testing and renamed to "gltf". Method RWGltf_GltfJsonParser::gltfParseSceneNode() is slightly refactored: parsing of transformations and transformation matrices is moved into separate functions.
141bd0a
to
bbbb8ea
Compare
Regression after Open-Cascade-SAS#184 where some methods are not isolated.
Regression after Open-Cascade-SAS#184 where some methods are not isolated.
Limited support of metadata import is added to RWGltf_GltfJsonParser. Following Json data types are currently supported: int32, double, string, array of int32, array of double, array of strings, Json object. Notable unsupported types are: binary data, array of Json objects. Metadata is processed for nodes and meshes.
Tests "gltf_export" are updated with import testing and renamed to "gltf".
Method RWGltf_GltfJsonParser::gltfParseSceneNode() is slightly refactored: parsing of transformations and transformation matrices is moved into separate functions.