Fix AudioNode issues when using velocityFromTranslation and small refactoring - #875
Conversation
There was a problem hiding this comment.
I'm not the author of AudioNode, but looking at the code, the velocityFromTranslation flag was supposed to change the behavior of the node during the update.
Now it's not anymore. So either it's relevant and the flag should be completely removed, and the setter getter deprecated, either it's an oversight and the flag should be added back. It's seems that the original issue is just a clonning issue.
Also the changes should follow the engines formatting scheme.
| super.updateGeometricState(); | ||
|
|
||
| if (channel < 0) { | ||
| if(channel<0||this.getParent()==null||!velocityFromTranslation) return; |
There was a problem hiding this comment.
Could you please follow the formatting scheme of the rest of the engine? This line should be
if (channel < 0 || this.getParent() == null || !velocityFromTranslation) return;
|
|
||
| if(!previousWorldTranslation.equals(currentWorldTranslation)){ | ||
| getRenderer().updateSourceParam(this,AudioParam.Position); | ||
| velocity.set(currentWorldTranslation).subtractLocal(previousWorldTranslation).multLocal(1f/lastTpf); |
There was a problem hiding this comment.
What about the velocityFromTranslation flag?
If it's not here it's now completely useless.
|
I moved the flag up to line 725, but on second thought this was going to skip the position update for nodes that do not use velocity from translation...
See rows 732 and 738 in the original code, if velocityFromTranslation is enabled, the first update will always turn velocity into NaN. I've made some changes, it should be fine now. |
Nehon
left a comment
Nehon
left a comment
There was a problem hiding this comment.
Ok good for me.
I must have missed the flag in previous review.
…actoring (jMonkeyEngine#875) * Fix AudioNode issues when using velocityFromTranslation and small refactoring * Fix audionode fix
|
Why does this have a getParent() == null check? That doesn't make sense to me... and seems like a big change in behavior (I almost never attach my AudioNodes to anything, for example). Definitely too risky for 3.2.2... and the check should maybe be removed. (Or at least explained.) |
|
I think the null check can be removed, tbh i don't remember if there is a reason for it to be there in the first place. |
|
Position worked before and now it doesn't, right. Velocity may have been broken before but position definitely wasn't... at least not when I've used it in the past. |
4 participants
This PR fixes some issues in the AudioNode that caused wrong velocity calculation when using velocityFromTranslation.
In short, when cloning the node, previousWorldTranslation wasn't reset to its initial value but cloned as reference, and the first velocity update was a subtraction with a NaN.
I've also modified the cloneFields method to reset the velocity to 0 if velocityFromTranslation is set, to prevent cloned nodes from preserving the velocity of the original in case they are never moved.