Issue 4387 gl tf extras #4571

Brijendra21 · 2022-04-19T12:53:27Z

Addition of Metadata and Userdata in the translated glTF

Fixes Replaces ThreeJS export with glTF export #4387

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…s in exported glTF

jmarrec · 2022-04-27T12:01:36Z

@Brijendra21 A description of your changes would be helpful. Also I'm pretty sure you can clean up the diff significantly. It seems some files are marked deleted instead of rename + couple of changes. Currently with 1 million lines added it's extremely hard to see what's going on

Brijendra21 · 2022-04-27T16:29:33Z

In this pull request we have made changes for glTF translator to include "metadata" and "userdata" to match what was in the threejs json file as Extras in glTF Schema.

Implementation Files :
Main:
src/model/GltfForwardTranslator.cpp
src/model/GltfForwardTranslator.hpp

Supporting:
src/model/GltfBoundingBoxWrapper.cpp
src/model/GltfBoundingBoxWrapper.hpp
src/model/GltfMetaDataWrapper.cpp
src/model/GltfMetaDataWrapper.hpp
src/model/GltfModelObjectMetadataWrapper.cpp
src/model/GltfModelObjectMetadataWrapper.hpp
src/model/GltfUserDataWrapper.cpp
src/model/GltfUserDataWrapper.hpp

Test Files:
GltfForwardTranslator_GTest.cpp
src/model/test/ThreeJSForwardTranslator_GTest.cpp

Showcasing Userdata and Metadata side by side (Threejs : glTF)

*Resources(*.osm & .glTF) which are added that are causing that excess difference.

tijcolem · 2022-04-29T15:13:29Z

@Brijendra21 Thanks for adding the additional info. I am seeing some build errors on the CI. You can click the links and check the errors. Here's a little snippet of some build errors (below). Check jenkins for full logs.

/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp: In member function ‘void openstudio::model::GltfMetaData::InitializeModelObject()’:
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:11: error: expected ‘(’ before ‘each’
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
           ^~~~
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:41: error: expected primary-expression before ‘modelObjectglTFMetaData’
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
                                         ^~~~~~~~~~~~~~~~~~~~~~~
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:11: error: ‘each’ was not declared in this scope
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
           ^~~~
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:11: note: suggested alternative: ‘path’
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
           ^~~~
           path

Also, to @jmarrec point. Could you explain a bit on each of the wrapper classes that you've implemented. It would be good to know how this functionality works so we can better maintain this going forward. I'd like to capture this and include it in our developer wiki docs.

jmarrec · 2022-05-01T16:02:25Z

@Brijendra21 The thing that bothers me with the excess diff is that I want to ensure it can't be avoided (perhaps it cannot). It seems a bunch of files were renamed and maybe slightly modified, but git is registering that as a full file deletion and one file addition.

For eg, what's the difference between RefBldgOutPatientNew2004_Chicago.gltf (former) and Sample_DOE-RefBldgOutPatientNew2004_Chicago.gltf (new) ?

…Data

ci-commercialbuildings · 2022-05-03T19:08:18Z

CI Results for c68e5db:

Brijendra21 · 2022-05-04T17:03:13Z

@Brijendra21 The thing that bothers me with the excess diff is that I want to ensure it can't be avoided (perhaps it cannot). It seems a bunch of files were renamed and maybe slightly modified, but git is registering that as a full file deletion and one file addition.

For eg, what's the difference between RefBldgOutPatientNew2004_Chicago.gltf (former) and Sample_DOE-RefBldgOutPatientNew2004_Chicago.gltf (new) ?

@jmarrec Yes we can avoid few of them if we decide to keep it in some other repository as a benchmark from the time of implementation. These are exported glTF files from various Tests and they rendered fine with 0 validation issues.
Prior version doesn't have UserData and MetaData but now they have both. Also we have added new *.osms as an input for the tests (created from Simergy Generated IDFs to fix few issues in the Geometry). So they are completely replaced.

Brijendra21 · 2022-05-04T17:18:47Z

@Brijendra21 Thanks for adding the additional info. I am seeing some build errors on the CI. You can click the links and check the errors. Here's a little snippet of some build errors (below). Check jenkins for full logs.

/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp: In member function ‘void openstudio::model::GltfMetaData::InitializeModelObject()’:
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:11: error: expected ‘(’ before ‘each’
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
           ^~~~
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:41: error: expected primary-expression before ‘modelObjectglTFMetaData’
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
                                         ^~~~~~~~~~~~~~~~~~~~~~~
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:11: error: ‘each’ was not declared in this scope
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
           ^~~~
/srv/jenkins/openstudio/git/incremental/OpenStudio/src/model/GltfForwardTranslator.cpp:796:11: note: suggested alternative: ‘path’
       for each (GltfModelObjectMetadata modelObjectglTFMetaData in glTFModelObjectMetadataVector) {
           ^~~~
           path

Also, to @jmarrec point. Could you explain a bit on each of the wrapper classes that you've implemented. It would be good to know how this functionality works so we can better maintain this going forward. I'd like to capture this and include it in our developer wiki docs.

@tijcolem These Wrapper Classes are only introduced as they're hiding the tinyglt::value as extras which is present in respective implementation of the same as Structure or Classes in Forward Translation Class and they serve the purpose of inferring the UserData Collection and MetaData against a loaded glTF model.
In one of the Tests I'm using them to do assertions against a glTF Model like this..

tijcolem · 2022-05-23T16:55:35Z

Looks good to me! Let's get this merged so users can begin exporting and viewing the data attributes on glTF.

This is an absolute minimal fix to have the tests passing, but really a refactor is warranted. The wrappers classes and superfluous, there's a lot of static storage objects throughout that should be deleting. The data layouts of the classes in GltfForwardTranslator makes little sense to me (using a tuple<string, T> makes little sense to me). cf #4571

This is an absolute minimal fix to have the tests passing, but I think we should refactor: * avoid all static storage objects * change the data layout and API in the classes that are in GltfForwardTranslator * just use `T` (eg double) instead of std::tuple<std::string, tinygltf::Value(T)> * provide a protected toGltfValue that will create the std::map<std::string, tinygltf::Value> * Move those to separate files (like the wrappers are) * Remove the wrappers (which are superflous now)The wrappers classes and superfluous, there's a lot of static storage objects throughout that should be deleting. The data layouts of the classes in GltfForwardTranslator makes little sense to me (using a tuple<string, T> makes little sense to me). cf #4571

Brijendra21 added 2 commits April 18, 2022 10:42

first draft for UserData & MetaData as Extras in Translated glTF

03d6580

some cleanup in gltTF Forward Translation

a3b7ea8

Brijendra21 requested a review from ggartside April 19, 2022 12:53

Brijendra21 added 4 commits April 20, 2022 11:22

part changes related to reinitializing glTFMetaData from Scene extras

caf9b6d

clang fomrat changes

40cf4e8

Updated Example Model Test to verify userdata and metadata from extra…

849431a

…s in exported glTF

clang format changes

4d9d575

Brijendra21 marked this pull request as ready for review April 26, 2022 17:50

Brijendra21 requested review from jmarrec and tijcolem April 26, 2022 17:51

tijcolem added the Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. label Apr 28, 2022

Brijendra21 added 2 commits April 29, 2022 09:37

Few changes to the for loops

1ef02aa

another small fix against the build error

9351294

Brijendra21 added 3 commits May 3, 2022 00:33

Added MetadataWrapper and UserdataWrapper in ModelGenerator.i

bf2fb0a

Added other depending classes in ModelGenerators.i

c83d9ac

Added typedef for vector of GltfModelObjectMetadataWrapper & GltfUser…

c68e5db

…Data

tijcolem approved these changes May 23, 2022

View reviewed changes

tijcolem merged commit 92b470f into develop May 23, 2022

tijcolem deleted the issue-4387_glTF_Extras branch May 23, 2022 16:56

jmarrec mentioned this pull request May 24, 2022

Hotfix GLTF extras broke running tests in Debug #4597

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 4387 gl tf extras #4571

Issue 4387 gl tf extras #4571

Brijendra21 commented Apr 19, 2022

jmarrec commented Apr 27, 2022

Brijendra21 commented Apr 27, 2022 •

edited

tijcolem commented Apr 29, 2022

jmarrec commented May 1, 2022

ci-commercialbuildings commented May 3, 2022 •

edited

Brijendra21 commented May 4, 2022 •

edited

Brijendra21 commented May 4, 2022

tijcolem commented May 23, 2022

Issue 4387 gl tf extras #4571

Issue 4387 gl tf extras #4571

Conversation

Brijendra21 commented Apr 19, 2022

Addition of Metadata and Userdata in the translated glTF

Pull Request Author

Review Checklist

jmarrec commented Apr 27, 2022

Brijendra21 commented Apr 27, 2022 • edited

tijcolem commented Apr 29, 2022

jmarrec commented May 1, 2022

ci-commercialbuildings commented May 3, 2022 • edited

Brijendra21 commented May 4, 2022 • edited

Brijendra21 commented May 4, 2022

tijcolem commented May 23, 2022

Brijendra21 commented Apr 27, 2022 •

edited

ci-commercialbuildings commented May 3, 2022 •

edited

Brijendra21 commented May 4, 2022 •

edited