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

Fix perspective matrices #117

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

swooster
Copy link

@swooster swooster commented Mar 19, 2021

(this is a followup to pull-request 89)

Here are some fixes to rh_ydown perspective matrices...

In order to make troubleshooting the unit tests easier, I had to improve eq_eps() diagnostics for narrow vectors. I've made changes to all vecs, despite only needing Vec3::eq_eps() to have clearer diagnostics; please let me know if there's another way I should handle that.

Also, to avoid wasted effort, this is currently limited to vulkan rh_ydown until I know what testing method is ok.

@fu5ha
Copy link
Owner

fu5ha commented May 21, 2021

Hey! So, I think we should rework EqualsEps a bit more and put the debug printing into an assert_eq_eps! macro instead of within the implementation of eq_eps itself. For inspiration/what I mean, see how I did it in colstodian recently:

https://github.com/termhn/colstodian/blob/main/src/lib.rs#L385-L471

Can also do a similar macro for eq_vk_corners/etc. if necessary

Other than that, this looks good.

@swooster swooster force-pushed the fix-perspective-matrices branch from 4b1fe09 to e69d20e Compare May 23, 2021 23:43
swooster added 2 commits May 23, 2021 16:44
To make that work, I had to alter eq_eps() to include an epsilon
parameter. Also, all direct uses of eq_eps() have been switched to
assert_eq_eps!().
@swooster swooster force-pushed the fix-perspective-matrices branch from e69d20e to f198285 Compare May 23, 2021 23:45
@swooster
Copy link
Author

Ok, I've tried to rework everything in terms of a colstodian-ish assert_eq_eps!() macro... For the perspective matrix stuff, I wasn't sure of the advantage of a macro so I just kept them as functions (though I slightly tweaked the names and implementation).

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