Rotate RenderInstance Translations if Created from a Gltf Template #7653
Conversation
andremig-bentley
requested review from
bbastings,
aruniverse,
ben-polinsky and
a team
as code owners
|
/azp run iTwin.js |
|
/azp run iTwin.js Docs - YAML |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run iTwin.js Integration - GitHub |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
core/frontend/src/tile/GltfReader.ts
Outdated
| @@ -2178,7 +2180,8 @@ export async function readGltfGraphics(args: ReadGltfGraphicsArgs): Promise<Rend | |||
| */ | |||
| export async function readGltfTemplate(args: ReadGltfGraphicsArgs): Promise<GltfTemplate | undefined> { | |||
| const baseUrl = typeof args.baseUrl === "string" ? new URL(args.baseUrl) : args.baseUrl; | |||
| const props = GltfReaderProps.create(args.gltf, true, baseUrl); // glTF supports exactly one coordinate system with y axis up. | |||
| const yAxisUp = args.yAxisUp ?? true; // default to true | |||
| const props = GltfReaderProps.create(args.gltf, yAxisUp, baseUrl); // glTF supports exactly one coordinate system with y axis up. | |||
There was a problem hiding this comment.
// glTF supports exactly one coordinate system with y axis up.
Per the comment, y is always up in glTF. Presumably the problem is in our attempt to align it to our z-up coordinate system. Caller should not have to fix that for us.
There was a problem hiding this comment.
In the latest commit, this has been addressed. A new rotation has been added to the instance's translation which is handled in createGraphicFromTeplate in System.ts. It is handled there so that we can check whether or not the template from which the instance is created is a gltf template. This way, the caller does not have to specify any option to fix this for us.
|
This pull request is now in conflicts. Could you fix it @andremig-bentley? 🙏 |
andremig-bentley
changed the title
|
Latest commit includes fixes to recreation of instances within |
| transforms[instanceIdx + 11] = -instanceY; | ||
| } | ||
|
|
||
| // recreate instances |
There was a problem hiding this comment.
Adding all these extra, redundant properties to RenderInstancesImpl so you can "recreate" the instances after computing incorrect transforms seems wrong. Can't you fix the transform earlier, and avoid having to recreate anything?
There was a problem hiding this comment.
A suggestion from looking around a bit, maybe this could be done in instanceGltfModel() in Instancing.ts, where it looks like the transform is initially calculated
Scratch that, somehow when I was doing a search I found that file but missed that it's example code...
There was a problem hiding this comment.
Unfortunately instanceGltfModel() is an example code snippet so I can't edit anything there to fix the issue. That being said, it does show the expected work flow of creating instances from a gltf template and it can show the issue I'm having here. Link to the function here.
The expected work flow for creating these instances is essentially:
1 - Obtain a Gltf template
2 - Create RenderInstances (including their transforms, important to this PR)
3 - Call createGraphicFromTemplate passing in the template and the instances.
Now we generally want to avoid making the caller pass in any information to tell us when to rotate the transforms. If we want to avoid this, we need to know that what we are instancing is a gltf template. This means that the earliest possible point to know if we need to rotate the transforms is when we also have access to the gltf template, so in createGraphicFromTemplate() which is what is being done here. Unfortunately, this means that they need to be recreated since at this point the instances' transforms are held in a BufferHandle in a readonly property of InstanceBuffersData.
This leaves to me, three options:
1 - Have the caller pass in a flag when creating the instances that tells us to rotate their transforms.
2 - Change InstanceBuffersData.transforms to be able to be written to, and then change the methods within createGraphicFromTemplate() to recreate that one BufferHandle and reassign it.
3 - Recreate the instances as we are doing here.
IMO, it makes the most sense to recreate the instances so that we can avoid relying on a caller to tell us to fix the issue, and so that we can avoid changing properties away from readonly. That being said, I think all three options could be valid solutions, and I'm open to any suggestions/discussion.
There was a problem hiding this comment.
2 is not an option because the instances can be shared by multiple graphics - if you mutate them you'll affect other graphics that are also using them.
This PR addresses this issue: #7454
When instancing an element from a gltf template, currently the instances will be placed incorrectly. This is due to gltf having a y-up axis orientation while our viewer has z-up. Currently, we are rotating the instance to be facing the correct direction, but the instance's translation is not oriented correctly. This PR fixes this issue by adding a rotation to the instance's translation within
createGraphicFromTemplateinSystem.ts. This rotation will only apply to instances created from a gltf template.