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

Matrix/Vector methods #118

Merged
merged 6 commits into from Apr 9, 2015
Merged

Matrix/Vector methods #118

merged 6 commits into from Apr 9, 2015

Conversation

brason
Copy link
Contributor

@brason brason commented Apr 9, 2015

Ok, so I added some more methods from three.js. I only found tests for compose/decompose, so If you think some tests are in order, I would need some help with that. Also, I feel that maybe Matrix4.compose should be a constructor instead of a method. What do you think?

@Fox32
Copy link
Contributor

Fox32 commented Apr 9, 2015

I would always prefer a method over a constructor. That allows to reuse an existing instance. On the other side, also adding a factory constructor calling that method wouldn't be a big deal and would satisfy everyone.

The same applies for methods like getNormalMatrix. I would prefer a copyNormalMatrix where I can supply a destination matrix. But providing both methods would be possible if getNormalMatrix calls copyNormalMatrix without much additional code.

About the apply* methods: Some of them are already available in Matrix as transform_. I'm not sure if we should have them both as apply_ and transform*. However, I also introduced perspectiveTransform some days ago and noticed later that there was already the right applyProjection method...

@brason
Copy link
Contributor Author

brason commented Apr 9, 2015

I guess you're right about reusing an exisiting instance. But if I were to create a constructor calling compose, what should it be called?

Also, I added copyNormalMatrix as you suggested.

Regarding the apply methods, I would prefer doing vector.applyMatrix4(matrix) over matrix.transform3(vector), but I don't see a problem having both options. The main reason I want to add applyMatrix3 and applyMatrix4 is because they are heavily used in three.js, and three.dart uses vector_math.

@Fox32
Copy link
Contributor

Fox32 commented Apr 9, 2015

The first moment when I saw compose and decompose I thought about fromTranslationRotationScale and copyTranslationRotationScale but these are quite long names =D

@brason
Copy link
Contributor Author

brason commented Apr 9, 2015

Yeah, those are pretty lengthy indeed :) Maybe we should leave it as is for now.

@Fox32
Copy link
Contributor

Fox32 commented Apr 9, 2015

By the way, Matrix4 has a method called setFromTranslationRotation so the name is not that far off.

johnmccutchan added a commit that referenced this pull request Apr 9, 2015
@johnmccutchan johnmccutchan merged commit 26065c5 into google:master Apr 9, 2015
@brason
Copy link
Contributor Author

brason commented Apr 9, 2015

Just as you know, moments after you merged I renamed the compose method to setFromTranslationRotationScale and created a Matrix4.compose constructor instead.

@johnmccutchan
Copy link
Collaborator

Hi,

Can you send me a pull request?

@brason
Copy link
Contributor Author

brason commented Apr 10, 2015

It seems like the change was included. I think I may have already pushed the change before you merged, but pushed again later to change the commit messages.

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.

None yet

3 participants