Skip to content

Conversation

@dvberkel
Copy link

@dvberkel dvberkel commented Aug 2, 2013

addresses issue #11

@dvberkel
Copy link
Author

dvberkel commented Aug 2, 2013

This pull request treats 2D vectors as 3D vectors with a z-component of 0.

Depending on the client this could break backwards compatibility.

Copy link
Owner

Choose a reason for hiding this comment

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

Why change !== to <? Isn't it also an issue if B.v.length > this.cols?

Copy link
Author

Choose a reason for hiding this comment

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

This is done to allow a 2x2 matrix operate on a 2D vector. Because this pull request treats 2D vectors as 3D vectors with a z-component 0, therefor B.v.length is 3.

So the 'simplest' solution to that problem was to allow matrices of any size act upon vectors just as long as there is a vector-element for each matrix-row, i.e. B.v.length >= this.cols.

Having said that, I agree that it is not intuitive. Because this is a consequence of treating 2D vectors as disguised 3D vectors, you can wonder if that is a good choice to do. There are alternatives.

@dvberkel
Copy link
Author

dvberkel commented Aug 4, 2013

@rockbot Do not forget that this pull request is a suggestion, and that an possible response is: "I do not like it, for these reasons".

@dvberkel dvberkel closed this Aug 4, 2013
@dvberkel dvberkel reopened this Aug 4, 2013
@dvberkel
Copy link
Author

dvberkel commented Aug 9, 2013

@rockbot I was wondering if you would like to see a different implementation or that you are ok with the one provided in this pull-request?

A different implementation would for example treat 2D vectors as vectors with 2 components and have a 'lift' method to lift them into a 3D vector.

What would you prefer?

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.

2 participants