-
Notifications
You must be signed in to change notification settings - Fork 26
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
update to raylib 5.5 #184
Conversation
@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.
|
I was able to run this locally on Linux! Nice work.
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.
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. |
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
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. |
update generated docs
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 .
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.
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 |
ok, no prob. I can do some testing. I just want to make sure we don't lose CI or Intel Mac support.
Yep. What I mean is I think we can leave both out, with
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.
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. |
@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 |
@huyhoang160593 one last thing from me: the force-push is using the incorrect github name. it should be |
README.md
Outdated
Tested only on ARM MacOS. | ||
|
There was a problem hiding this comment.
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.
Tested only on ARM MacOS. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
more about this:
not all the examples were updated (for example here.) I can do this after, though. I think whatever we do, it should be consistent. |
Ok, 1 more thing: so on the apple CI change it copies it to 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 Running M1 on Inel, I got this error:
So I think this needs a closer look. I think cmake can do it with 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. |
…ort on example to using relative path
I have make the changes to all example to keep the consistent in the lib, and remove the fail document
I also delete the error workflow as you mention too, we should look at this in separate pr later |
Looks good to me! |
I added a PR here about code-formatting. |
I also add this PR to do mac CI better. |
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