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 compile on Linux (attempt 2) #37

Merged
merged 4 commits into from
Jul 15, 2022

Conversation

joppiesaus
Copy link
Contributor

@joppiesaus joppiesaus commented Jun 29, 2022

I fixed it by changing the type to int32_t, instead of the suggested uint32_t, as glfwGetWindowSize(GLFWwindow* window, int* width, int* height); uses signed ints, therefore it seems to it makes more sense to change it to an int32_t. Plus less things to change to the codebase. Cheers!

(See also #36)

an uint32_t was implicitely casted to a int32_t.
Fixed by changing the type to int32_t.
@W2Wizard W2Wizard added the Fix Fixed or resolved an issue label Jun 30, 2022
@W2Wizard
Copy link
Collaborator

The old PR was totally fine btw, you could have done this change in there but oh well.

@joppiesaus
Copy link
Contributor Author

I have changed a bunch of stuff. It should be looked at thoroughly. It changes some definitions, int32_ts are changed in favor of uint32_t. This might break existing projects that use MLX42, as the resize hook is also changed, although the fixes should be very simple to apply. Furthermore, one can call mlx_set_window_size with an uint32_t1 above MAX_INT but below unsigned max int value, and it will flip the signs around. Though to be honest, I don't think there are monitors with a size of 2,147+ billion. Same said for mlx_get_monitor_size.

@W2Wizard W2Wizard merged commit 8a3c63d into codam-coding-college:master Jul 15, 2022
W2Wizard added a commit that referenced this pull request Jul 15, 2022
This reverts commit 8a3c63d, reversing
changes made to 481a763.
@W2Wizard
Copy link
Collaborator

Thought it should just work right out of the gates, instead everything broke. So for now I reverted it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Fixed or resolved an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants