Skip to content

[osg] Add Collada model support to OpenSceneGraph#3950

Merged
Codiferous merged 2 commits intomicrosoft:masterfrom
calumr:osg-collada
Feb 11, 2019
Merged

[osg] Add Collada model support to OpenSceneGraph#3950
Codiferous merged 2 commits intomicrosoft:masterfrom
calumr:osg-collada

Conversation

@calumr
Copy link
Contributor

@calumr calumr commented Jul 25, 2018

Collada models are .dae files, and are the only open format you can download from 3dwarehouse.sketchup.com.

@Codiferous
Copy link
Contributor

@calumr Sorry for the tremendous delay in processing this PR -- we're focusing very heavily on processing PRs now!

Is the fix-virtual-method.patch patchfile necessary here? I tried building this port without it and everything seemed fine. In general, we try to avoid intrusive patches to libraries in vcpkg in order to void deviating from the library's functionality. If the library needs to be significantly patched, we prefer that the project takes the changes as a PR.

Would this PR be something we could accept without the virtual method patch?

@calumr
Copy link
Contributor Author

calumr commented Feb 7, 2019

The patch was a hack to work around a crash in daeIOPluginCommon::beginReadElement. When you try to load a .dae file in OSG, it crashes in here and I didn't get the time to debug the issue. I can't recall why I tried making the methods non-virtual, but it worked.

I don't have time to investigate a fix right now. If possible I'd prefer to merge without the fix-virtual-method.patch and I will come up with a proper fix when my team is finished our current release.

I could try attaching a minimal test case that reproduces the bug?

@calumr
Copy link
Contributor Author

calumr commented Feb 7, 2019

Actually I think this is just a case of missing -DCOLLADA_DOM_SUPPORT150 out of the osg build. I'll submit a patch soon.

@Codiferous Codiferous self-assigned this Feb 7, 2019
@Codiferous
Copy link
Contributor

Okay, this PR is looking pretty good to me! Let's get it merged 😄 Thanks for this, and sorry again for the significant delay in processing this!

@Codiferous Codiferous merged commit 10bde77 into microsoft:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants