Added VelocityVectorProperty #3908
Conversation
|
@hpinkos can you review this? |
|
|
||
| var property = this._position; | ||
| if (Property.isConstant(property)) { | ||
| return undefined; |
There was a problem hiding this comment.
Wouldn't this be the zero vector instead of undefined?
There was a problem hiding this comment.
Well, I guess if you wanted the vector to always be normalized this makes sense. Just wanted to double check.
There was a problem hiding this comment.
The other properties seem to return undefined when there isn't a value at that time. This code was pretty much copied from the VectorOrientationProperty. @mramato is returning undefined the right thing to do here?
There was a problem hiding this comment.
Yes, undefined just gets translated to "use the default" which is probably the best case scenario here.
|
@tfili can we rewrite |
| expect(property.isConstant).toBe(true); | ||
| expect(property.definitionChanged).toBeInstanceOf(Event); | ||
| expect(property.position).toBeUndefined(); | ||
| expect(property.getValue(time)).toBe(undefined); |
There was a problem hiding this comment.
.toBe(undefined) -> .toBeUndefined()
|
@tfili just had that one question |
|
@mramato this should be good for another look. |
| * @param {Property} [position] The position property used to compute the velocity. | ||
| * | ||
| * @example | ||
| * //Create an entity with position and orientation. |
There was a problem hiding this comment.
This example is now incorrect. I would recommend updating it to illustrate orienting a billboard along it's velocity vector.
There was a problem hiding this comment.
The comment is still incorrect. Change it to something like //Create an entity with a billboard rotated to match its velocity
|
Just that one last comment. |
|
@mramato Updated the code sample. |
|
Also, please merge in master. |
|
@mramato Master is merged and the comment is fixed. |
|
Thanks @tfili |
Added
VelocityVectorPropertyso billboard's aligned axis can follow the velocity vector. Tests were also added.