-
Notifications
You must be signed in to change notification settings - Fork 74
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
support referenced content extension #913
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.
mostly looks pretty good, I want to talk about how this could make an easy hop to supporting a glbRepresentation type, see if that desirable, and maybe change some language to support that.
Reviewed all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)
Elements/src/Serialization/glTF/GltfExtensions.cs
line 1223 at r1 (raw file):
var nodeId = NodeUtilities.CreateNodeForMesh(meshId, nodes, content.Id, transform); if (contentElement != null && UseReferencedContentExtension)
contentElement != null should already be true at this point.
Elements/src/Serialization/glTF/GltfExtensions.cs
line 1283 at r1 (raw file):
{ AddExtension(gltf, nodes[nodeId], "HYPAR_referenced_content", new Dictionary<string, object> { {"contentUrl", contentElement.GltfLocation}
I think we should change the language here away from contentURL towards glbURL.
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.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)
Elements/src/Serialization/glTF/GltfExtensions.cs
line 1223 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
contentElement != null should already be true at this point.
Done.
Elements/src/Serialization/glTF/GltfExtensions.cs
line 1283 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
I think we should change the language here away from contentURL towards glbURL.
I'm not sure I agree. One reason I don't want to do this is that the format for the referenced content may not always be a GLB. Because we're no longer doing any merging in the function, we can now easily do referenced content of any type threeJS can display: Rhino files, point clouds, objs, etc.
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.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann)
Elements/src/Serialization/glTF/GltfExtensions.cs
line 1223 at r1 (raw file):
Previously, andrewheumann wrote…
Done.
I don't see it.
Elements/src/Serialization/glTF/GltfExtensions.cs
line 1283 at r1 (raw file):
Previously, andrewheumann wrote…
I'm not sure I agree. One reason I don't want to do this is that the format for the referenced content may not always be a GLB. Because we're no longer doing any merging in the function, we can now easily do referenced content of any type threeJS can display: Rhino files, point clouds, objs, etc.
yup, I buy that. ContentElements are dead long live Content!
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.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @wynged)
Elements/src/Serialization/glTF/GltfExtensions.cs
line 1223 at r1 (raw file):
Previously, wynged (Eric Wassail) wrote…
I don't see it.
forgot to push oops!
Code quote:
if (contentElement != null && UseReferencedContentExtension)
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained
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.
Reviewed 1 of 2 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained
BACKGROUND:
DESCRIPTION:
Elements.Serialization.glTF.GltfExtensions.UseReferencedContentExtension = true;
is on.TESTING:
This change is