Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added VelocityVectorProperty #3908

Merged
merged 6 commits into from
May 16, 2016
Merged

Added VelocityVectorProperty #3908

merged 6 commits into from
May 16, 2016

Conversation

tfili
Copy link
Contributor

@tfili tfili commented May 10, 2016

Added VelocityVectorProperty so billboard's aligned axis can follow the velocity vector. Tests were also added.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 11, 2016

@hpinkos can you review this?


var property = this._position;
if (Property.isConstant(property)) {
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be the zero vector instead of undefined?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess if you wanted the vector to always be normalized this makes sense. Just wanted to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, undefined just gets translated to "use the default" which is probably the best case scenario here.

@mramato
Copy link
Contributor

mramato commented May 11, 2016

@tfili can we rewrite VelocityOrientationProperty in terms of this now? I imagine it would get rid of all of the code duplication.

expect(property.isConstant).toBe(true);
expect(property.definitionChanged).toBeInstanceOf(Event);
expect(property.position).toBeUndefined();
expect(property.getValue(time)).toBe(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.toBe(undefined) -> .toBeUndefined()

@hpinkos
Copy link
Contributor

hpinkos commented May 11, 2016

@tfili just had that one question

@tfili
Copy link
Contributor Author

tfili commented May 12, 2016

@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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is now incorrect. I would recommend updating it to illustrate orienting a billboard along it's velocity vector.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is still incorrect. Change it to something like //Create an entity with a billboard rotated to match its velocity

@mramato
Copy link
Contributor

mramato commented May 12, 2016

Just that one last comment.

@tfili
Copy link
Contributor Author

tfili commented May 13, 2016

@mramato Updated the code sample.

@mramato
Copy link
Contributor

mramato commented May 13, 2016

Also, please merge in master.

@tfili
Copy link
Contributor Author

tfili commented May 16, 2016

@mramato Master is merged and the comment is fixed.

@mramato mramato merged commit 62d63f7 into master May 16, 2016
@mramato mramato deleted the velocity-vector-property branch May 16, 2016 16:04
@mramato
Copy link
Contributor

mramato commented May 16, 2016

Thanks @tfili

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants