Entity's transformation matrix is the source of truth #23
Conversation
|
@michalbe I think this is finally ready for you review. |
| if (this.vertices && this.indices && this.normals) { | ||
| this.create_buffers(); | ||
| } | ||
| } | ||
|
|
||
| look_at(vec, up = this.up) { |
There was a problem hiding this comment.
Should we call this target_to?
There was a problem hiding this comment.
I don't think so - look_at is self explanatory, I believe in this case we're having it better than gl-matrix. Keep in mind that Unity's look_at is doing exactly what ours is doing.
| math.mat4.target_to(this.local_matrix, this.position, vec, up); | ||
| } | ||
|
|
||
| get left() { |
There was a problem hiding this comment.
The current coordinate system (Y = up) is left-handed, so it makes sense to rename right to left. Note that this isn't used anywhere else in the code right now.
There was a problem hiding this comment.
Doooooo we need to keep this vector then? Since you've get rid of Entity::realign and all the computations are handled without it, I believe it became useless. Do you?
| math.mat4.from_rotation(rotation_matrix, rad, vec); | ||
| math.mat4.translate(this.local_matrix, this.local_matrix, this.origin); | ||
| math.mat4.multiply(this.local_matrix, this.local_matrix, rotation_matrix); | ||
| math.mat4.translate(this.local_matrix, this.local_matrix, reverse_origin); |
There was a problem hiding this comment.
The order of the translations here is reversed compared to the current v1.0. I didn't get to test this yet, so this might be wrong.
There was a problem hiding this comment.
I also think that we should remove origin for now: the same effect can be achieved with parents. It's unclear to me right now if children's origin should be used for rotating them if the parent is rotated.
There was a problem hiding this comment.
I don't fully understand your concerns here, maybe it's because origin isn't the most fortunate name for this attribute.
Lemme visualise my idea behind it.
Default origin
cube.origin = [0, 0, 0];
game.add_frame_listener('do_stuff', delta => {
cube.rotate_rl(16/1000);
});the cube's rotating in place.
Non-default origin
cube.origin = [ -2, 0, 0];
game.add_frame_listener('do_stuff', delta => {
cube.rotate_rl(16/1000);
});The entity is still rotating, but the point of rotation (the origin) is displaced:
Of course you can achieve the same thing with a parent entity, but it increases number of elements in scene, number of computations per frame, and is probably harder to use than just changing yet another attribute on the class.
There was a problem hiding this comment.
Thanks for providing more information about this. I filed #35.
| -1, 0, 1, | ||
| 1, 0, 1, | ||
| 1, 0, -1, | ||
| -1, 0, -1 |
There was a problem hiding this comment.
In the new XYZ coordinates, the plane was rendered vertically rather than horizontally.
| if (this.vertices && this.indices && this.normals) { | ||
| this.create_buffers(); | ||
| } | ||
| } | ||
|
|
||
| look_at(vec, up = this.up) { |
There was a problem hiding this comment.
I don't think so - look_at is self explanatory, I believe in this case we're having it better than gl-matrix. Keep in mind that Unity's look_at is doing exactly what ours is doing.
| math.mat4.target_to(this.local_matrix, this.position, vec, up); | ||
| } | ||
|
|
||
| get left() { |
There was a problem hiding this comment.
Doooooo we need to keep this vector then? Since you've get rid of Entity::realign and all the computations are handled without it, I believe it became useless. Do you?
| math.mat4.from_rotation(rotation_matrix, rad, vec); | ||
| math.mat4.translate(this.local_matrix, this.local_matrix, this.origin); | ||
| math.mat4.multiply(this.local_matrix, this.local_matrix, rotation_matrix); | ||
| math.mat4.translate(this.local_matrix, this.local_matrix, reverse_origin); |
There was a problem hiding this comment.
I don't fully understand your concerns here, maybe it's because origin isn't the most fortunate name for this attribute.
Lemme visualise my idea behind it.
Default origin
cube.origin = [0, 0, 0];
game.add_frame_listener('do_stuff', delta => {
cube.rotate_rl(16/1000);
});the cube's rotating in place.
Non-default origin
cube.origin = [ -2, 0, 0];
game.add_frame_listener('do_stuff', delta => {
cube.rotate_rl(16/1000);
});The entity is still rotating, but the point of rotation (the origin) is displaced:
Of course you can achieve the same thing with a parent entity, but it increases number of elements in scene, number of computations per frame, and is probably harder to use than just changing yet another attribute on the class.
Entity's transformation matrix is the source of truth
Entity's transformation matrix is the source of truth
Still WIP, but already functional.