Skip to content

Add base support for vec6#583

Merged
justadudewhohacks merged 8 commits into
justadudewhohacks:masterfrom
GlenTiki:feat/vec5
Jun 25, 2019
Merged

Add base support for vec6#583
justadudewhohacks merged 8 commits into
justadudewhohacks:masterfrom
GlenTiki:feat/vec5

Conversation

@GlenTiki

@GlenTiki GlenTiki commented Jun 23, 2019

Copy link
Copy Markdown
Contributor

As per issue #578 this library needs support for vec6. This is a PR to add that support.

Comment thread cc/core/Vec.cc
info[3]->ToNumber(Nan::GetCurrentContext()).ToLocalChecked()->Value(),
info[4]->ToNumber(Nan::GetCurrentContext()).ToLocalChecked()->Value(),
info[5]->ToNumber(Nan::GetCurrentContext()).ToLocalChecked()->Value()
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanted to rewrite this using macros to minimise the duplication but I'm too unfamiliar in cpp land to do it correctly, so this is the alternative I came up with.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

No worries, no need to introduce macros here :)

@GlenTiki GlenTiki changed the title WIP: add base support for vec6 Add base support for vec6 Jun 23, 2019
@justadudewhohacks

Copy link
Copy Markdown
Owner

Great thanks for the PR, looks good to me! Just a few minor things. :)

Could you add the typings for Vec6, similar to Vec4.d.ts.

Furthermore, since you added support for Vec6 in the cv.partition binding, could you also add the typing here and the testcase here,

@GlenTiki

Copy link
Copy Markdown
Contributor Author

Nice, feedback is incorporated now! :)

While in there I noticed that Vec4's typing was wrong for the constructor, so I fixed it.

@justadudewhohacks

Copy link
Copy Markdown
Owner

Great, thanks!

@justadudewhohacks justadudewhohacks merged commit 4fb9333 into justadudewhohacks:master Jun 25, 2019
@GlenTiki GlenTiki deleted the feat/vec5 branch June 25, 2019 15:40
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