Skip to content

update to raylib 5.5 #184

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

Merged
merged 12 commits into from
Feb 16, 2025
Merged

update to raylib 5.5 #184

merged 12 commits into from
Feb 16, 2025

Conversation

huyhoang160593
Copy link
Contributor

Combine with the update of r1tsuu and some tweak on CMake file, now Window build will be working as intended. And some minor change on import node module to prevent further conflict with package with same name

@konsumer
Copy link
Collaborator

konsumer commented Feb 15, 2025

@huyhoang160593 thanks for this. Lots of good changes here.

Many are cosmetic (like using double-quotes instead of single) we really should get an actual style def in here. Personally, I use standardjs for a lot of things, since it's not mutable (there is only 1 standard-style) is very readable to me (I am dyslexic, and semi-colons at end, and double-quotes are less readable for js) and fairly automatic, but if we want something different, I recommend prettier to lock-in the choices. @RobLoach do you have any prefs for code-style? Prettier is automatic in VSCode, Zed, and Subl (with a plugin) So I often use a standard-like-config and for most things it just fixes the code when I save. This can smooth out a lot of style changes.

I see a couple things I would like to know more about.

  • apple silicone should be handled, if compiled on mac (since mac clang does this by default with "fat binaries".) Did you test it, and it had problems? I have ARM and intel macs to test on, if we need to. On github runners, when I set it up, they used intel macs, so the filename in runner should have x86 (even though it supports both.) Has this changed?
  • set(CMAKE_BUILD_TYPE Release) should set optimization flags. Was the juggling of -O2/-O3 needed?
  • I prefer require('raylib') to require("../../index.js") in demos. We did this with a mapping somewhere, and it makes them look more like they would for users. I do like using node: for built-in requires, though. both are valid, but it is more clear.
  • can you explain the key differences to codegen? It seems like just the versions and a few other things were changed, but it's so much code-diff, it's hard to tell with some of the structures/functions

@RobLoach
Copy link
Owner

I was able to run this locally on Linux! Nice work.

@RobLoach do you have any prefs for code-style?

No preference, though we could likely bring some code-styles fixes in as follow-up pull requests. As long as it's consistent, I'm happy. All the code style changes does make it rather difficult to code review.

I prefer require('raylib') to require("../../index.js") in demos

While I understand making it easier to understand, I believe having it explicitly using the internal relative paths makes it really clear that it's using the local copy rather than one that may or may not be installed in node_modules.

@konsumer
Copy link
Collaborator

konsumer commented Feb 15, 2025

No preference, though we could likely bring some code-styles fixes in as follow-up pull requests. As long as it's consistent, I'm happy. All the code style changes does make it rather difficult to code review.

Yep, that is my issue. Like we all have different styles so small things look big. I will add this prettier config after the PR is resolved, and we can tune the config, if anyone hates the standard-style. We can go even further to test style-conformance in CI, which cuts down on a lot of style changes, but is also very annoying. It can sort of force devs to run prettier --fix (or let their editor do that on save) but also might not really be necessary, if everyone just follows it.

While I understand making it easier to understand, I believe having it explicitly using the internal relative paths makes it really clear that it's using the local copy rather than one that may or may not be installed in node_modules.

Yes, I hear that. sounds good.

Personally, this gets a 👍 from me. I think we can apply the style-change after, and I want to look closer at the window/mac specific changes to build (since I think both of these are done automatically) but overall it seems great, and these are minor nitpicks, and really could just be things to explore in other commits.

I'd also like to get @r1tsuu 's feedback, if possible, since it looks like a lot of the changes came from their repo.

@huyhoang160593
Copy link
Contributor Author

apple silicone should be handled, if compiled on mac (since mac clang does this by default with "fat binaries".) Did you test it, and it had problems? I have ARM and intel macs to test on, if we need to. On github runners, when I set it up, they used intel macs, so the filename in runner should have x86 (even though it supports both.) Has this changed?

This was previous handle by @r1tsuu and I'm just expand on that, and I'm also not have a mac myself so this is hard for me to test .

set(CMAKE_BUILD_TYPE Release) should set optimization flags. Was the juggling of -O2/-O3 needed?

They are only set at -O3 from the git history, but on Window MVSC, the O3 flag doesn't exist so I change this to O2, doing this mostly for the compile process not warning with unknow flag.

can you explain the key differences to codegen? It seems like just the versions and a few other things were changed, but it's so much code-diff, it's hard to tell with some of the structures/functions

The change mostly happen with the update of @raylib/api from @RobLoach so I think for the codegen, this should be fine for the most part. With caveat case, we can create another PR to fix them later

And lastly, for code style, especially with js, I recommend to use biome cause of the unify of styling tool, as biome combine most the goody of eslint and prettier in one single tools, we also have a automatic format with extention and if the codebase grow larger, the biome also have better speed

@konsumer
Copy link
Collaborator

konsumer commented Feb 16, 2025

This was previous handle by @r1tsuu and I'm just expand on that, and I'm also not have a mac myself so this is hard for me to test .

ok, no prob. I can do some testing. I just want to make sure we don't lose CI or Intel Mac support.

They are only set at -O3 from the git history, but on Window MVSC, the O3 flag doesn't exist so I change this to O2, doing this mostly for the compile process not warning with unknow flag.

Yep. What I mean is I think we can leave both out, with set(CMAKE_BUILD_TYPE Release) It's fine either way, though, I can explore this after PR is merged. I may be wrong about this, but I thought that was part of what made it Release.

The change mostly happen with the update of @raylib/api from @RobLoach so I think for the codegen, this should be fine for the most part. With caveat case, we can create another PR to fix them later

Agreed. I think "mostly 5.5 but needs some minor tweaks" is better than "4.x". I have used @raylib/api in other codegen, and it seems good at 5.5, so there should not be too many probs.

And lastly, for code style, especially with js, I recommend to use biome cause of the unify of styling tool, as biome combine most the goody of eslint and prettier in one single tools, we also have a automatic format with extention and if the codebase grow larger, the biome also have better speed

Prettier can both format and report problems (and eslint can too.) Since prettier is built into more editors, I use it a bit more, personally, but they all seem about the same to me. It's just a standard though, for example I use Zed, which has a rust-implementation of prettier that is much faster than the js version. I do like that biome is faster than standard prettier, but from their page, it seems like it can even use the same config, so maybe standardizing on prettier makes more sense. Looks like zed actually uses biome. What I am proposing here is not any particular tool, just that, after this PR, we check-in a .prettierrc file, so people know how to format their code, and editors can do it automatically.

@huyhoang160593
Copy link
Contributor Author

@konsumer for the better I think we also need a automate CI build for formatting too. Cause not everybody will remember to run the formatting, but the CI will alway going for those step

@konsumer
Copy link
Collaborator

@huyhoang160593 one last thing from me: the force-push is using the incorrect github name. it should be RobLoach/node-raylib. I left a comment about it.

README.md Outdated
Comment on lines 1 to 2
Tested only on ARM MacOS.

Copy link
Owner

Choose a reason for hiding this comment

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

Worked on Ubuntu over here.

Suggested change
Tested only on ARM MacOS.

Copy link
Collaborator

@konsumer konsumer Feb 16, 2025

Choose a reason for hiding this comment

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

I tested on these and it built/ran:

  • x86_64 fedora
  • x86_64 mac
  • ARM (M1) mac

I still think we need to double-check CI (since there were changes to mac build) to make sure it's building the pre-builds correct on Github, but it looks good to me, otherwise. Not sure about windows, and it's a bit harder for me to test there, maybe @twuky can take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@konsumer my local machine is running on window so I can confirm the build is working on my end

@konsumer
Copy link
Collaborator

more about this:

While I understand making it easier to understand, I believe having it explicitly using the internal relative paths makes it really clear that it's using the local copy rather than one that may or may not be installed in node_modules.

not all the examples were updated (for example here.) I can do this after, though. I think whatever we do, it should be consistent.

@konsumer
Copy link
Collaborator

konsumer commented Feb 16, 2025

Ok, 1 more thing:

so on the apple CI change it copies it to node-raylib-darwin-arm64.node but in the same file it does another build and copies to node-raylib-darwin-x64.node. Now, keep in mind on github, it only builds on intel-mac, so I think this would either name an ARM build incorrectly, or it's a redundant build (the same file could be copied to both.) I think the reason this worked was that apple clang automatically makes "fat binaries" by default that work for both. I think a better fix would be to not use the CPU on mac in postinstall and build for both with 1 (node-raylib-darwin.node.) This would allow the original prebuild CI-config to work for both. This is another fix I can do after (it doesn't block mac use, just doesn't help it) but wanted to call it out, so we don't forget.

We definitely need to test that the same binary works on both mac arches, but last I checked, they are interchangable.

For ref, and easier cross-checking, here are some builds I just made (copy into build/Release/node-raylib.node):

Running M1 on Inel, I got this error:

build/Release/node-raylib.node' (no such file), '/Users/konsumer/Desktop/node-raylib/build/Release/node-raylib.node' (mach-o file, but is an incompatible architecture (have 'arm64', need 'x86_64h' or 'x86_64'))

So I think this needs a closer look. I think cmake can do it with CMAKE_OSX_ARCHITECTURES:STRING=x86_64;arm64, but as it is, the CI change for ARM will not work, because Github runs mac stuff on Intel-based emulators.

As far as I know, this could just be added to the global cmake:

set(CMAKE_OSX_ARCHITECTURES arm64 x86_64)

and then, we will need to remove the new mac ARM CI stuff, and setup postinstall to use a single arch. I can do this as a separate PR, though, it's not blocking this.

@huyhoang160593
Copy link
Contributor Author

@konsumer

not all the examples were updated (for example here.) I can do this after, though. I think whatever we do, it should be consistent.

I have make the changes to all example to keep the consistent in the lib, and remove the fail document

and then, we will need to remove the new M1 CI stuff, and setup postinstall to use a single arch. I can do this as a separate PR, though, it's not blocking this.

I also delete the error workflow as you mention too, we should look at this in separate pr later

@konsumer
Copy link
Collaborator

Looks good to me!

@konsumer
Copy link
Collaborator

I added a PR here about code-formatting.

@konsumer
Copy link
Collaborator

I also add this PR to do mac CI better.

@RobLoach RobLoach merged commit 983e73f into RobLoach:master Feb 16, 2025
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.

4 participants