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

add I16Vec and U16Vec #439

Merged
merged 4 commits into from
Nov 24, 2023
Merged

add I16Vec and U16Vec #439

merged 4 commits into from
Nov 24, 2023

Conversation

kawogi
Copy link
Contributor

@kawogi kawogi commented Nov 23, 2023

In response to #260 I gave it a try to add the shorter vector types (I16Vec_ and U16Vec_).

I still need to expand some of the test cases, but vec2 is complete and looks good already.

I wasn't able to test all targets (esp. cuda) and I had to guess the alignments for cuda. Also there might be other places where the alignment isn't correct due to wrong assumptions. Somebody please have an eye on that.

Otherwise I added a few missing From/TryFrom conversions.

Feedback is welcome, so I know whether to invest more time in the remaining tests.

Edit: I just found the test runner for the features. Will do some fixing …

@kawogi
Copy link
Contributor Author

kawogi commented Nov 23, 2023

build_and_test_features.sh shows all green now.

Only the tests for vec3 and vec4 are missing and I'm still unsure whether the CUDA alignments are correct.

Copy link
Owner

@bitshifter bitshifter left a comment

Choose a reason for hiding this comment

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

Nice, thanks for all of your work on this!

@bitshifter bitshifter merged commit 03c2233 into bitshifter:main Nov 24, 2023
15 checks passed
@kawogi
Copy link
Contributor Author

kawogi commented Nov 25, 2023

Thanks for the crate!

Are the test cases for vec3/vec4 still required?

@bitshifter
Copy link
Owner

Ah yes that's a good point, coverage did drop a bit with this PR (see https://coveralls.io/builds/64166568), I forgot to look. It would be good if you could add those missing tests.

@kawogi
Copy link
Contributor Author

kawogi commented Nov 26, 2023

I actually added the missing test cases (and some more) on my fork (https://github.com/kawogi/glam-rs/tree/main/src) but after wasting over an hour I couldn't figure out how to make a new PR from that state! I'm totally lost with GIT(Hub) right now. Might try tomorrow again. Any help is appreciated.

It looks like my fork didn't include the recent changes from your repo before making my changes, but I thought that https://github.com/kawogi/glam-rs/pull/1 was supposed to do that.

@bitshifter
Copy link
Owner

When a PR is merged the default behaviour in github is to squash the changes (you can see the squashed commit here 03c2233). This means that the history gets changed, so if you continue to work on the same branch your original changes are on then git will think they aren't related.

When making a PR you should first create a local branch, make your changes, commit the branch, push it to github and make the PR from that. Once the PR is merged you can delete the branch. Since your changes are on main, it can be a bit tricky, you might be able to rename your main branch, then checkout the current main from upstream glam to a new main so your main is correct. Then create a new branch and either file copy or git cherry pick the changes you made from the old main. If this is too much hassle let me know and I can try cherry pick the changes you made from your fork.

@kawogi
Copy link
Contributor Author

kawogi commented Nov 26, 2023

Thanks a lot for your explanation and I think I understood the problem. For now, I cherry-picked the changes on my own into a new fork but will remember your suggestions for the next time.

Follow-up: #440

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