enginmanap / limonEngine

3D FPS game engine with full dynamic lighting and shadows
http://www.limonengine.com
GNU Lesser General Public License v3.0
580 stars 57 forks source link

Animation Interface CalculateTransform is unintuitive #54

Closed enginmanap closed 6 years ago

enginmanap commented 6 years ago

The method returns Transform object, but have a parameter "isFound". If that is required, it should return isFound, and take the transform as in/out parameter

nickshillingford commented 6 years ago

Hello,

I'd like to take this issue but i'm wondering if you could clarify a little more about what CalculateTransform should do. If it's supposed to take a Transformation as a parameter instead of a nodeName string, what would we be searching for in the nodes map? Since it holds AnimationNodes and not Transformations.

The only thing I can think of is to check if the given Transformation's vec3 matches the vec3 of any of the AnimationNodes in the map. If so, perform setScale, setOrientation and setTranslate, then return "isFound" ?

enginmanap commented 6 years ago

Hi,

the method is this:

https://github.com/enginmanap/limonEngine/blob/bfa2ca28b621fb0ca9306efdcab3501eb78d49a7/src/Assets/Animations/AnimationInterface.h#L15

This is not a functional requirement, but a refactoring. The method works as expected, but I believe the method should be like this:

virtual bool calculateTransform(const std::string& nodeName, float time, Transformation& tranformation ) const = 0;

The return value should be status, and transform should be passed as argument.

nickshillingford commented 6 years ago

Ahh okay. I was just wondering because you have this comment above the method in AnimationAssimp.cpp:

//FIXME requiring node name should not be a thing

Thanks for clarifying :)

enginmanap commented 6 years ago

That is because the same interface is used for 2 cases: 1) Animation for whole body 2) Animation for single node

I don't have a task for it because I don't think about it to come up with a elegant solution. If you have one, please feel free to open a new ticket for it.

enginmanap commented 6 years ago

fixed by @nickshillingford