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 return type of emscripten_glfw_is_window_fullscreen #5

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

sbc100
Copy link
Contributor

@sbc100 sbc100 commented Jun 27, 2024

I'm trying to change the type of EM_BOOL and this is currently blocking that:
emscripten-core/emscripten#22155

I'm trying to change the type of EM_BOOL and this is currently
blocking that:
  emscripten-core/emscripten#22155
Copy link
Member

@ypujante ypujante left a comment

Choose a reason for hiding this comment

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

I don't understand the change you are making. You are trying to get rid of EM_BOOL in emscripten, yet this PR changes int to EM_BOOL. What am I missing?

@ypujante
Copy link
Member

The problem is that your fix is not changing include/GLFW/emscripten_glfw3.h where it is defined...

@ypujante
Copy link
Member

@sbc100 I think I understand what you are doing (which you could have explained...)

You changed (but KEPT) the define from int to bool

But my code in glfw3.cpp is using int instead of using EM_BOOL and that causes the issue you are seeing...

I will patch it...

@ypujante ypujante merged commit f4ce142 into pongasoft:master Jun 27, 2024
@sbc100
Copy link
Contributor Author

sbc100 commented Jun 27, 2024

Yeah its not important (to me) what the return type of this function is.. as long as the header and source agree.

@sbc100 sbc100 deleted the fix_rtn_type branch June 27, 2024 18:15
@sbc100
Copy link
Contributor Author

sbc100 commented Jun 27, 2024

Would you mind sending a PR to emscripten to update the port? (or should I just do it as part of my PR?)

@ypujante
Copy link
Member

@sbc100 I am already in the process... should take a few minutes

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