remove OPENCOLLADA_ANIMATION_CLIP compile flag
AbandonedPublic
Actions

Authored by SushiKitty (SushiKitty) on Jan 26 2022, 4:35 AM.

Details

Reviewers

Ray Molenkamp (LazyDodo)
Gaia Clary (gaiaclary)

Summary

The OPENCOLLADA_ANIMATION_CLIP is supposed to turn on if it detects certain versions of OpenCOLLADA; however, this failed on my machine:

D:\workspace\blender\source\blender\io\collada\collada.cpp(64,20): error C2259: 'DocumentImporter': cannot instantiate abstract class [D:\workspace\build_windows_x64_vc17_Releas
e\source\blender\io\collada\bf_collada.vcxproj]
D:\workspace\blender\source\blender\io\collada\DocumentImporter.h(47): message : see declaration of 'DocumentImporter' [D:\workspace\build_windows_x64_vc17_Release\source\blende
r\io\collada\bf_collada.vcxproj]
D:\workspace\blender\source\blender\io\collada\collada.cpp(64,20): message : due to following members: [D:\workspace\build_windows_x64_vc17_Release\source\blender\io\collada\bf_
collada.vcxproj]
D:\workspace\blender\source\blender\io\collada\collada.cpp(64,20): message : 'bool COLLADAFW::IWriter::writeAnimationClip(const COLLADAFW::AnimationClip *)': is abstract [D:\wor
kspace\build_windows_x64_vc17_Release\source\blender\io\collada\bf_collada.vcxproj]
D:\workspace\lib\win64_vc15\opencollada\include\opencollada\COLLADAFramework\COLLADAFWIWriter.h(115): message : see declaration of 'COLLADAFW::IWriter::writeAnimationClip' [D:\w
orkspace\build_windows_x64_vc17_Release\source\blender\io\collada\bf_collada.vcxproj]

despite having setup dependencies according to Blender documentation with CMake and made sure I have the latest git checkout and latest svn checkout of the dependencies.

Removing the flag allows compilation to succeed. I believe this flag was originally used to support multiple versions of OpenCOLLADA. The library itself hasn't been updated for many years both upstream and in the bf-blender svn repository, so I don't think there is anymore need for this flag to be present to support older versions of OpenCOLLADA.

The last release was Nov 26, 2018: https://github.com/KhronosGroup/OpenCOLLADA/releases

Let me know if this is a reasonable change. This is my first time contributing to Blender and also first time contributing to a C++ project.

Thank you

Diff Detail

Repository: rB Blender

Event Timeline

SushiKitty (SushiKitty) requested review of this revision.Jan 26 2022, 4:35 AM

SushiKitty (SushiKitty) created this revision.

SushiKitty (SushiKitty) edited the summary of this revision. (Show Details)Jan 26 2022, 4:37 AM

SushiKitty (SushiKitty) edited the summary of this revision. (Show Details)

SushiKitty (SushiKitty) added inline comments.Jan 26 2022, 4:41 AM

source/blender/io/collada/DocumentImporter.h
133	Copied from https://svn.blender.org/svnroot/bf-blender/trunk/lib/win64_vc15/opencollada/include/opencollada/COLLADAFramework/COLLADAFWIWriter.h

SushiKitty (SushiKitty) added reviewers: Ray Molenkamp (LazyDodo), Gaia Clary (gaiaclary).Jan 26 2022, 5:28 PM

I'm a little divided here, you're not wrong, the lib hasn't been updated in ages, and removing support for even older version wouldn't necessarily be a bad thing
however.... it failing to find COLLADAFWAnimationClip.h which hasn't been a problem for anyone in literally years _is_ a little suspicious and deserves a closer
look.

can you validate win64_vc15\opencollada\include\opencollada\COLLADAFramework\COLLADAFWAnimationClip.h exists in your lib folder?

In D13923#370161, @Ray Molenkamp (LazyDodo) wrote:

can you validate win64_vc15\opencollada\include\opencollada\COLLADAFramework\COLLADAFWAnimationClip.h exists in your lib folder?

It exists, which is why I found it to be a weird problem. It could be that I was compiling with an incomplete svn checkout since it was taking a long time, but it failed even after I finished the checkout. I then started the environment from scratch, and now it works again and now I can't reproduce the original problem.

The issue might have been a red-herring from a combination of incomplete checkout + some cached state that's not invalidated after checkout is finished. I'm not super familiar with CMake so I don't know whether there's any caching mechanism at play that might enable this hypothesis.

Either way, I can now build without this patch on latest git/svn checkouts. It might still be worth doing to remove support for older OpenCOLLADA versions.

Thank you for the review,

The issue might have been a red-herring from a combination of incomplete checkout + some cached state that's not invalidated after checkout is finished. I'm not super familiar with CMake so I don't know whether there's any caching mechanism at play that might enable this hypothesis.

it'll be a guessing game, but the only way i could see this breakage persist is

At the time cmake ran and generated the project files COLLADAFWAnimationClip.h did not exist and put the code in the "old collada mode"
You realized the lib folder was incomplete and addressed the issue
CMake did not run after that and hasn't updated the project files to inform the compiler collada actually IS the "new' version.

When you use our make.bat helper it'll make sure cmake runs before every build, but if you are using the IDE that may or may not always happen.

Given this patch now fixes problems for no-one, and may break something for those few users that may still have an ancient collada version, i'm going to play it safe and not land it, thank you for the work you put in though!

no problem. thanks for the review anyways