search

janet-lang / jaylib

Janet bindings to Raylib
MIT License
137 stars 36 forks source link

Partial update to Raylib 4.0.0 #32

Closed sogaiu closed 2 years ago

sogaiu commented 2 years ago

This PR mostly contains changes to update to Raylib 4.0.0.

It isn't a complete update but rather attends to:

for what already exists in jaylib.

One existing jaylib test was changed to remove the use of draw-text-rec as the corresponding Raylib function DrawTextRec was removed in Raylib 4.0.0.

On a side note Raylib's CHANGELOG file was consulted to verify that most of the changes were expected.

There is also a change for removing a duplicate draw-grid -- this is unrelated to updating to Raylib 4.

saikyun commented 2 years ago

Great work!

Just a tiny suggestion, I think GetRenderTextureTexture2d is a bit long, could it be named RenderTextureToTexture / render-texture->texture? :)

sogaiu commented 2 years ago

I agree it's long.

I don't know about renaming though. Here are some concerns:

May be that hasn't been adhered to in places?

However, I don't know that it's not doable to have an additional name for something that's long.

In any case, may be it makes sense to discuss this kind of thing in an issue (i.e. not this PR)? I am not a fan of overly long names, but I think it might be good to examine the issue a bit more in-depth (or have someone put their foot down :) )

saikyun commented 2 years ago

Oh, I thought it was a name you added. Never mind that then. Imo it's a bit confusing that it's named "texture2d" when all other things returning textures are called "texture". But if it was already there, then it has nothing to do with this PR, so please ignore my comment. :)

Den lör 5 mars 2022 kl 23:46 skrev sogaiu @.***>:

I agree it's long.

I don't know about renaming though. Here are some concerns:

  • The C and Janet names have been there from before https://github.com/janet-lang/jaylib/blob/d337eac39a55d0d2fe3996bafe8ad3a9efdef1a7/src/image.h#L660

  • Without some kind of general pattern, I'm not so sure looking stuff up doesn't get complicated for folks who don't know the names yet. ATM, IIUC, bakpakin described the naming scheme as:

    The bindings are faithful to the original C API, especially where it makes sense, but function names have been "lispified" (kebab case, question marks instead of the word "Is", etc.). May be that hasn't been adhered to in places?

However, I don't know that it's not doable to have an additional name for something that's long.

In any case, may be it makes sense to discuss this kind of thing in an issue (i.e. not this PR)? I am not a fan of overly long names, but I think it might be a good to examine the issue a bit more in-depth (or have someone put their foot down :) )

— Reply to this email directly, view it on GitHub https://github.com/janet-lang/jaylib/pull/32#issuecomment-1059844766, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAS46Z6UGJ5VPD4ZTAHGUS3U6PP53ANCNFSM5O2JOGNA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

saikyun commented 2 years ago

(For context, that function isn't needed in C since then you just look at the property .texture.)

Den mån 7 mars 2022 kl 10:13 skrev Jona Ekenberg @.***>:

Oh, I thought it was a name you added. Never mind that then. Imo it's a bit confusing that it's named "texture2d" when all other things returning textures are called "texture". But if it was already there, then it has nothing to do with this PR, so please ignore my comment. :)

Den lör 5 mars 2022 kl 23:46 skrev sogaiu @.***>:

I agree it's long.

I don't know about renaming though. Here are some concerns:

  • The C and Janet names have been there from before https://github.com/janet-lang/jaylib/blob/d337eac39a55d0d2fe3996bafe8ad3a9efdef1a7/src/image.h#L660

  • Without some kind of general pattern, I'm not so sure looking stuff up doesn't get complicated for folks who don't know the names yet. ATM, IIUC, bakpakin described the naming scheme as:

    The bindings are faithful to the original C API, especially where it makes sense, but function names have been "lispified" (kebab case, question marks instead of the word "Is", etc.). May be that hasn't been adhered to in places?

However, I don't know that it's not doable to have an additional name for something that's long.

In any case, may be it makes sense to discuss this kind of thing in an issue (i.e. not this PR)? I am not a fan of overly long names, but I think it might be a good to examine the issue a bit more in-depth (or have someone put their foot down :) )

— Reply to this email directly, view it on GitHub https://github.com/janet-lang/jaylib/pull/32#issuecomment-1059844766, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAS46Z6UGJ5VPD4ZTAHGUS3U6PP53ANCNFSM5O2JOGNA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

saikyun commented 2 years ago

I've been using sogaiu's PR for a couple of weeks, and I think it works great.

If this PR could be merged, I could make a PR for rlgl functions I've wrapped too (translation, rotation and scaling).

sogaiu commented 2 years ago

The Update raylib (e6a394e) was a mistake.

The subsequent force-push from e6a394e to 63b8362 was for restoring things to the previous state.