jmonkeyengine
jmonkeyengine copied to clipboard
jMonkeyEngine
New animation system javadoc
The docs, since I unfortunately wrongly disrupted my master branch with some forks, no changes are made to the already committed files since the last PR, so you can continue revising from where you have stopped.
@stephengold thanks for reviewing, give me sometime to read those requested changes and see.
I have made the requested changes for the Action class, and rephrased some parts, hope its better now, please review them.
I have cleaned up BlendableAction before being reviewed to minimize time reviewing it.
@stephengold Could you please review the Action and BaseAction classes again ?
I am really sorry for these stupid mistakes, i will try to omit these mistakes as possible on the other un-revised classes.
Thanks for your patience.
I'd like to finish the review of the Action and BaseAction first then move on to other classes.
I am really sorry for these stupid mistakes
None of the mistakes I've seen so far are "stupid".
Be gentle on yourself. You've assigned yourself a daunting task. Documenting someone else's code is always a challenge. The new animation system is particularly obscure. And English is (I think) a language especially unsuited to writing technical documentation.
The good news is that your skills are improving. You're already better at writing documentation than you were a few weeks ago. I hope you see that.
After you've made the changes you promised, I'll make another review pass.
@Scrappers-glitch Are you still available to work on this PR?
Yeah ofc, I have not left the project, but I am currently have ongoing exams that will extend up to two weeks.
This is still on my todo, give me a few months, i will free somethings out from my stack and then return for those requested changes, i have a better suggestion for this, may be i will close this PR and separate the work into multiple organized PRs based on the packages, we will work on each package separately and i will make sure our conversation here is not wasted out by picking up the important requested changes !
As a general comment, every time I pick up this animation system again I feel forced to relearn all of its weird concepts from scratch again... usually be reading the code and finding working examples. While I appreciate the effort that has gone into the documentation so far, I'm not sure it's really alleviated that problem for me.
It's unfortunate that nehon never gave us many bread crumbs because I'm sure the author would have had insights into what the reasoning was that we have to reverse engineer.
That said, I'm not sure what the solution really is, either. The javadoc should help tell us how to use these classes with some high level explanations but we somehow miss this mark. It always takes me a lot of time to re-wrap my head around it and so I can't really provide much help on how to push it closer to that goal.
...so in the end, my comments are not helpful, I guess.
Put another way, the javadoc for these classes should be like stereo instructions. They should tell me how to plug things together, what the impedance ranges are so I don't blow something up, what the knobs do, etc.. I don't really need circuit diagrams and chip documentation.
First and foremost, the javadoc should be answering "why" and "when" do I use a particular class. "How" do I set it up to do that? What are the limitations I can expect? The details of internal workings are for internal code comments for someone trying to change the class later. Javadoc is for the users. Dumb it down a lot.
Edit: and if someone ever comes up with a really good explanation about the difference between actions and anim clips and the weird wiring between them, I will owe them several beers. That's such a confusing thing I have to relearn every time... and even more frustrating because one of those things is not persisted in a j3o.
Edit2: and yes, too late I realize that "stereo instructions" may not resonate with the whole age range of this audience. Perhaps video game console would have been a better metaphor.
and if someone ever comes up with a really good explanation about the difference between actions and anim clips and th
The only thing I know currently about this from the code, and some words from the original author on forums is that AnimClip is where the original animation data are wrapped, while Actions is where the porting of Tween to the animation system came from, Nehon has built an abstraction layer over Tweens and didn't use the Tweens API directly, I think that is a flaw in the design and maybe it's a residual part from the old State-Machine pattern, but maybe I am wrong, maybe it supplies out some good code tricks that wouldn't have been if he used the Tween API directly, I have to really look back in forums to understand more as I weren't there during these days.
EDIT:
And, I think the wiring between both occurs over the AnimLayer and the implementation used for the Action class.
EDIT2:
Another observation to obtain that might support my description of the Action class hierarchy, is that the BlendableAction + Action is almost identical to the AbstractTween ... I have no guess rather than the Action class hierarchy is a bridging mechanism in reality between the Tween API and the new Animation System and it was a residual API that was based on a State-Machine pattern, but now moves to the Tween.
My two cents:
I think the current flaw in the design is due to having a backward playback (negative speed) and the other thing is that the animation mask is integrated into the animation layer (the mask is passed by the anim layer to the action). Because of those the Action class needs to "visit" the passed tweens (that can be chained) and extract the actions (ClipAction and BlendActions).
To solve those, for backward playback, we can use an Invert tween and instead of passing mask by layer, we can require the user to set the mask on the action when creating the action.
Now we should be able to remove BaseAction and Action classes and just keep the BlendableAction, ClipAction, and BlendAction. (we can rename them if required.)
And the next step could be to remove the AnimLayer and instead use TweenAnimation and AnimationState (imported from Lemur) to run animations. For this, we should refactor AnimComposer to use "channels" like the Lemur EfectControl I think.
difference between actions and anim clips and the weird wiring between them
AnimClip (IMO could be renamed to AnimData or ClipData), contains the animation track data which is savable to j3o whereas the ClipAction (can not think of a good name atm) is the Tween ("runtime" thing) that can not be saved and it uses the anim data.
I think the current flow in the design is due to having a backward playback (negative speed) and the other thing is that the animation mask is integrated into the animation layer (the mask is passed by the anim layer to the action). Because of those the Action class needs to "visit" the passed tweens (that can be chained) and extract the actions (ClipAction and BlendActions).
To solve those, for backward playback, we can use an Invert tween and instead of passing mask by layer, we can require the user to set the mask on the action when creating the action.
Now we should be able to remove BaseAction and Action classes and just keep the BlendableAction, ClipAction, and BlendAction. (we can rename them if required.)
And the next step could be to remove the AnimLayer and instead use TweenAnimation and AnimationState (imported from Lemur) to run animations. For this, we should refactor AnimComposer to use "channels" like the Lemur EfectControl I think.
It seems a good idea, though it smells of re-designing the whole system or parts of it and re-designing a system without understanding some of its parts might break things out.
Out of innovation, we may try this on a fork or a new API before thinking to change the current API, and I am ready to participate in this, it will be a great learning experience!
I hope there won't be a de novo new Animation System after this :-).
Out of innovation, we may try this on a fork or a new API before thinking to change the current API, and I am ready to participate in this, it will be a great learning experience!
That sounds a good idea.
Also, I think "speed" should not be an attribute on the tween/action but it should be an attribute on the thing that "runs" the tween. (i.e. AnimLayer or TweenAnimation)
Out of innovation, we may try this on a fork or a new API before thinking to change the current API, and I am ready to participate in this, it will be a great learning experience!
That sounds a good idea.
Also, I think "speed" should not be an attribute on the tween/action but it should be an attribute on the thing that "runs" the tween. (i.e. AnimLayer or TweenAnimation)
After documenting the base API, I will open a forum thread to discuss this as a community project so we can reach a better solution.