Bring tinygltf in as a dependency #21521
Conversation
SeanCurtis-TRI
added
the
release notes: feature
|
+(release notes: feature) To see how this inevitably gets used, see #21520. |
There was a problem hiding this comment.
+@jwnimmer-tri for feature review, please.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
SeanCurtis-TRI
force-pushed
the
PR_tinygltf
branch
from
36c0f36 to
f16f8fc
Compare
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers
tools/workspace/tinygltf_internal/repository.bzl line 9 at r1 (raw file):
name = name, repository = "syoyo/tinygltf", commit = "e0b393c6958c0a7cbe134a240fad7915aae53db3",
When possible, we prefer numbered releases instead of git sha. Are we able to use v2.8.22 here, and do we ancitipate upstream release tags to appear on a frequent enough basis to suit us?
tools/workspace/tinygltf_internal/patches/function_pointer.patch line 6 at r1 (raw file):
This doesn't bind *all* function pointer typedefs, just the ones that Drake cares about. We should consider replacing all function pointers and submitting an upstream patch.
BTW Yes, please add this to the overall issue's checklist. I'd like us to submit this patch (or an improved version) upstream while it's still fresh in our heads.
tools/workspace/tinygltf_internal/patches/vendor.patch line 1 at r1 (raw file):
Add vendoring namespace.
BTW Does our cc_library_vendored( choke on this file? If that's the reason we're writing out the patch manually, we should probably say so in a comment here.
It vendors the header and does a minimal smoke test in RenderEngineGl's
unit tests.
In addition to vendoring:
- we redirect tinygltf to use Drake's vendored json library.
- We change the callback declaration from a C-style function pointer
to a std::function to allow a callback with captures.
SeanCurtis-TRI
force-pushed
the
PR_tinygltf
branch
from
f16f8fc to
a2b8df8
Compare
There was a problem hiding this comment.
+@rpoyner-tri for platform review, please.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
tools/workspace/tinygltf_internal/repository.bzl line 9 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
When possible, we prefer numbered releases instead of git sha. Are we able to use
v2.8.22here, and do we ancitipate upstream release tags to appear on a frequent enough basis to suit us?
Done
On the basis he's put out ten releases in the last year (with the most recent being just two weeks ago), it seems it's under lively development.
tools/workspace/tinygltf_internal/patches/function_pointer.patch line 6 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Yes, please add this to the overall issue's checklist. I'd like us to submit this patch (or an improved version) upstream while it's still fresh in our heads.
I've updated the issue and submitted the PR to tinygltf.
tools/workspace/tinygltf_internal/patches/vendor.patch line 1 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Does our
cc_library_vendored(choke on this file? If that's the reason we're writing out the patch manually, we should probably say so in a comment here.
I hadn't been aware of it. :) Cargo culted from the wrong thing.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
tools/workspace/tinygltf_internal/repository.bzl line 9 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Done
On the basis he's put out ten releases in the last year (with the most recent being just two weeks ago), it seems it's under lively development.
Ah good, thanks. (I hadn't actually looked; those were just the stock questions about whether to use tags vs head.)
tools/workspace/tinygltf_internal/patches/vendor.patch line 1 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I hadn't been aware of it. :) Cargo culted from the wrong thing.
Sorry :(
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
tools/workspace/tinygltf_internal/repository.bzl line 9 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah good, thanks. (I hadn't actually looked; those were just the stock questions about whether to use tags vs head.)
No worries. I hadn't thought of looking at the releases at all. So, this was a helpful exercise all around.
tools/workspace/tinygltf_internal/patches/vendor.patch line 1 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Sorry :(
No apologies necessary. Again, I learned. The tricky bit was injecting tinygltf into the visibility list for the vendor tool. The message was a bit confusing because we have a build target vendor_cxx and vendor_cxx.bzl and vendor_cxx.py (which is associated with the build target). I wasted a few minutes wondering why vendor_cxx.bzl had visibliity settings. :-/
There was a problem hiding this comment.
Reviewed 5 of 10 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform)
It vendors the header and does a minimal smoke test in RenderEngineGl's
unit tests.
In addition to vendoring:
- we redirect tinygltf to use Drake's vendored json library.
- We change the callback declaration from a C-style function pointer
to a std::function to allow a callback with captures.
It vendors the header and does a minimal smoke test in RenderEngineGl's unit tests.
In addition to vendoring:
Relates #20185.
This change is