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

Functions added to Rectangle, and less verbose enums #50

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

LiamM32
Copy link
Contributor

@LiamM32 LiamM32 commented Mar 27, 2024

Resolves #8
I experimented with using CTFE to generate new versions of the enums. Realizing that I can't add aliases to existing enum members this way, I ended up just directly writing shorter identifiers in package.d alongside the existing overly-verbose ones. I left the CTFE function in there just in case you want to see.

I also added functions to Rectangle for convenience. These are to get the corners of the rectangle as a Vector2. There are also operator overloads for offsetting a rectangle by a Vector2.

Originally, I was going to include some D ports of the official Raylib examples, before seeing the existing repository for these examples. Perhaps we should consider merging them into here, with a format that can be found in previous commits on my repository in the examples directory.

If I can figure out a way to generate them, I may later make a commit to add overloads of some functions. (#48)

@schveiguy
Copy link
Owner

Hi Liam, thanks for the PR. I'm not ignoring you, I just haven't had any time to look at this kind of stuff.

I promise I will take a look at some point...

@@ -103,7 +103,7 @@ int main() {
//----------------------------------------------------------------------------------

// Custom GUI font loading
//Font font = LoadFontEx("fonts/rainyhearts16.ttf", 12, 0, 0);
//Font font = LoadFontEx("fonts/rainyhearts16.ttf".toStringz, 12, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

D string literals are compatible with C strings. This line works as-is in D.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have found that sometimes it works, and sometimes it doesn't. As an experiment, I just tried removing all the .toStringz uses in my code, and the only errors were for the functions MeasureTextEx and DrawTextEx. Having experienced this error before, I had added this use of toStringz for peace of mind, thinking that it might just suddenly stop working one day without it.

Do you know why using a D string works for some Raylib functions but not for others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just figured it out. It works without toStringz when the string is known at compile-time, but fails when a variable is used. I will now keep this in mind.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, any string literals are implicitly convertible to const char * and will have proper zero termination. That is a part of the D language specification: https://dlang.org/spec/expression.html#string_literals

So please remove this change.

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 7, 2024

@schveiguy While the features added in this fork are so far rather small, I plan to make some more commits soon. If you don't merge it very soon, the number of changes are likely to pile up, and there will be more for you to review.

Do you need me to put the new changes on another branch with a separate pull request?

If you don't have time to review merge requests, perhaps you should add repo permission for an experienced D programmer who you trust with this.

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 8, 2024

I have some updates to share on what I have worked on. I haven't committed everything that I have tried, as I didn't want to make any commits that cause Raylib-D to to stop functioning, so much of it has remained on my own computer. However, you can see look in the string-overloads branch to see my almost-working attempt to add something new. I will explain what I have been trying to do on this branch, and the problems I faced.

The new thing I intended to add are overloads of existing Raylib functions which take D strings instead of C strings as arguments. A difficulty with this was that D string literals are already accepted by the existing C functions. This meant that when the functions were called using a string literal, the compiler would complain that the call matches both versions of the function. The solution I found was to change string in the function parameters to ref string, so that literals would go to the old function. However, this sometimes caused errors that I didn't know how to explain.

Most of the function overloads I added are generated using a CTFE function, which uses string concatenation to generate the new functions. The RayGUI example currently has betterC in the build options. Unfortunately, this results in string concatenation not being allowed; not even during compile-time function evaluation! This means that a build error would happen when building with betterC. It reports use of "TypeInfo" being the problem in the build output, but Rikki Cattermole told me that string concatenation is the problem. A temporary solution I added was to make the new overloads unavailable in the betterC builds using version (D_TypeInfo).

I just became aware of the betterC build option. I wonder how much benefit it adds. Would it be best to just remove it from the Raygui example? Right now I just made two separate build configurations for it; one with betterC and one without.

After all this trouble, I decided to make an attempt to simply port some Raylib functions into D. This is in the port-functions branch. I know that @schveiguy has a "draylib" repository to port Raylib into D, but I think that instead of porting all of it, a better near-term goal would be to only port functions that involve strings, while not being equivalent to any Phobos functions. I already ported "DrawText" and "DrawTextEx" with good results from the little testing I have done.

Stopped using the CTFE function to generate the enum.
@schveiguy
Copy link
Owner

Do you need me to put the new changes on another branch with a separate pull request?

So sorry for being late on this. I'll take a look tonight to see what you have done here. I know not having feedback is frustrating.

I just became aware of the betterC build option. I wonder how much benefit it adds.

This was requested by a user on the discord. Given that raylib-d is a simple wrapper around a C library, it makes sense that betterC is achievable (and it was with a couple of insignificant changes).

If there are things that you are using that are not betterC compatible, you can add them with version(D_BetterC) else {/* your code here */}. It would make sense that if you want to use a more D-like experience, you should have to eschew using betterC.

Copy link
Owner

@schveiguy schveiguy left a comment

Choose a reason for hiding this comment

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

So I like a lot of the stuff here. The enum changes are not exactly what I was looking for, and I think you should split this into a separate PR which we can discuss separately. The Rectangle functions are awesome, and I have wanted them many many times.

The enum changes might work this way, but you need to make it very simple to generate new bindings, which I don't think this does. Keep in mind, all the binding files that come from raylib are automatically generated using the instructions in generating.md. If you make any changes to these files by hand, those changes need to be outlined in that document, and "replace all cases of KEY_A with A" is not a good instruction -- I spend about 1-2 hours on generating the bindings when a release occurs, and I depend on the simplicity of the instructions for making sure everything is properly done. I simply don't have the cycles to test that every translation is correct, or hand-adjust all the enums in the file.

What I was looking for when I wrote #8 is something that provides an alternative to the existing enums (imported separately), not replacing the enums in the original file. In other words, the generated enum should be the "better" one.

@@ -103,7 +103,7 @@ int main() {
//----------------------------------------------------------------------------------

// Custom GUI font loading
//Font font = LoadFontEx("fonts/rainyhearts16.ttf", 12, 0, 0);
//Font font = LoadFontEx("fonts/rainyhearts16.ttf".toStringz, 12, 0, 0);
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, any string literals are implicitly convertible to const char * and will have proper zero termination. That is a part of the D language specification: https://dlang.org/spec/expression.html#string_literals

So please remove this change.

result.y -= offset.y;
}
return result;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Oooh, I like all these! Very Nice.

source/raylib/raylib_types.d Outdated Show resolved Hide resolved
source/raylib/raylib_types.d Outdated Show resolved Hide resolved
source/raylib/raylib_types.d Outdated Show resolved Hide resolved
source/raylib/raylib_types.d Outdated Show resolved Hide resolved
source/raylib/raylib_types.d Outdated Show resolved Hide resolved
"license": "Zlib",
"name": "example"
"name": "example",
"mainSourceFile": "source/app.d"
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't be necessary.

@schveiguy
Copy link
Owner

schveiguy commented Apr 10, 2024

Unfortunately, this results in string concatenation not being allowed; not even during compile-time function evaluation!

There is one trick to make a function CTFE only -- assign the result of an immediately-called lambda to an enum:

string notGoodForBetterC()(string x, string y) {
    string result;
    result ~= x;
    result ~= ", ";
    result ~= y;
    return result;
}

enum thisDoesntWork = notGoodForBetterC("hello" , "world!");

enum thisDoesWork = (string x, string y) {
    string result;
    result ~= x;
    result ~= ", ";
    result ~= y;
    return result;
}("hello", "world");

Applied most of the suggested changes, but excluded one which appears to be wrong.

Co-authored-by: Steven Schveighoffer <[email protected]>
@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 10, 2024

So I like a lot of the stuff here. The enum changes are not exactly what I was looking for, and I think you should split this into a separate PR which we can discuss separately. The Rectangle functions are awesome, and I have wanted them many many times.

What I was looking for when I wrote #8 is something that provides an alternative to the existing enums (imported separately), not replacing the enums in the original file. In other words, the generated enum should be the "better" one.

Right. I understood that, but giving them the same name is my preference. My reasoning for putting the better enums directly in the file, and generating the old ones is to encourage use of the better ones, since they can easily be seen by someone reading the file.

I found D's restrictions on enum definitions to be a huge pain. It would be so great if additional members of the same enum could be defined in different places, but it's not allowed. Further, mixins can't be placed in enum definitions. This makes things hard.

Added `Rectangle.centre` function.
Deprecated some symbols which are deprecated in mainline Raylib.
@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 10, 2024

I've been thinking of turning raygui into a subpackage, so that both 3.5 and 4.0 will later be available in separate configurations.

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 10, 2024

The enum changes might work this way, but you need to make it very simple to generate new bindings, which I don't think this does. Keep in mind, all the binding files that come from raylib are automatically generated using the instructions in generating.md.

Do you think it's likely that existing enums will change?

If not, perhaps all the enums can be placed in a separate file, and the instructions can be to remove them from package.d.

@schveiguy
Copy link
Owner

Do you think it's likely that existing enums will change?

Yes

https://github.com/raysan5/raylib/pull/3733/files

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 12, 2024

Do you think it's likely that existing enums will change?

Yes

https://github.com/raysan5/raylib/pull/3733/files

Well, that's just the old "menu" button on Android, which modern Android doesn't even use. But I may have a solution that can work even if enum values change.

Perhaps we can have a dedicated script to write the enum definitions based on the current Raylib version. I don't know if CtoD can be configured for specialized purposes like this, but I think the easier solution would be to use std.json to read the enum information from raylib/parser/output/raylib_api.json, and place them in their own file. I don't think this is beyond my skill level to do.

I can probably write a script on my own that can write automatically generate such enums automatically, with and without the prefixes. The one thing that may be beyond my skill level is if I want it to maintain any alternative names for enum members that were added manually.

@schveiguy
Copy link
Owner

Well, that's just the old "menu" button on Android, which modern Android doesn't even use

It's just an example. The thing is, raylib changes stuff all the time, adding enum values, removing them, changing them, etc.

Perhaps we can have a dedicated script to write the enum definitions based on the current Raylib version

Something that is automated is OK, but I am very wary of a fragile tool that needs updating when things break.

I'm still confused why we can't just generate the new enums instead of editing the file? They will still be visible to the user.

Something like:

/// map a new enum `Key` such that `KeyboardKey.KEY_A` => `Key.A`
mixin(GenerateBetterEnum!(KeyboardKey, "Key", "KEY_"));

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 12, 2024

I'm still confused why we can't just generate the new enums instead of editing the file? They will still be visible to the user.

Something like:

/// map a new enum `Key` such that `KeyboardKey.KEY_A` => `Key.A`
mixin(GenerateBetterEnum!(KeyboardKey, "Key", "KEY_"));

Well, I originally wrote that function thinking of just that. But now I think we would be better off using the names of the existing enums. Unfortunately, D requires all enums to be defined in only one place, so we can't do alias KeyboardKey.A = KeyboardKey.KEY_A; in a separate file.

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 12, 2024

Check out this new updater program I wrote and placed in a new branch. Right now it just generates the enums and places them in a separate module, but this approach can potentially be used to generate the rest of the bindings.

I'll wait for your feedback before editing generating.md.

@schveiguy
Copy link
Owner

I am OK adding anything that is not a direct translation of C code in a separate module, as long as the binding modules are generated, and look similar to the existing system.

We can't remove existing symbols (unless raylib removes them). We can't change the meaning of them. At its heart, this is a binding with some D niceties added onto it. It should be usable as a binding first. Someone who has code that says

import raylib;

KeyboardKey k = KeyboardKey.KEY_A;

it should continue to compile.

If you can produce a tool that is effective at generating these separate modules, then I'm fine adding it, as long as the core binding isn't affected. raylib-d can grow new features and niceties, but it must retain the existing features and API.

But I'd recommend at this point, separating out these ideas into a separate PR, and we can add the Rectangle methods now.

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 12, 2024

But I'd recommend at this point, separating out these ideas into a separate PR, and we can add the Rectangle methods now.

I would do this, but I don't know how, aside from putting a commit to revert all files except raylib_types.d back to the version on your repo, but I prefer not to do that. Is there a way I can make this pull request only one file?

@schveiguy
Copy link
Owner

So I also am not an expert on git. My usual solution is to just to create new branch and copy in the changes I want.

One thing that is unfortunate is that you have made these changes on your master branch. You should always checkout a new branch when proposing changes to an upstream repository.

This is my recommendation:

# save the original file you want to keep
cp source/raylib/raylib_types.d x.d
# create a branch so this doesn't get lost
git checkout -b savemystuff
# make sure it gets onto github
git push origin savemystuff
# delete your local master branch
git branch -d master
# make sure the upstream remote is present (optional, if you haven't done it yet)
git remote add upstream https://github.com/schveiguy/raylib-d.git
# checkout the upstream master
git checkout upstream/master
# overwrite your master branch on github.
git push origin master --force
# checkout a branch to target with this PR
git checkout -b updaterect
# copy the code you kept
cp x.d source/raylib/raylib_types.d
# commit and push this branch (edit commit message)
git commit -a
# push to your github
git push origin updaterect

and now, you can go onto github and change the source branch to this new branch (instead of master)

@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 12, 2024

I am OK adding anything that is not a direct translation of C code in a separate module, as long as the binding modules are generated, and look similar to the existing system.

Are you against putting anything that's not strictly a binding to a raylib symbol inside package.d?

Unfortunately, this would really limit us, as D requires that enums are defined in only one place, and all overloads of a function are defined in the same module. I think it would be optimal to have less verbose names for enum members while still giving the enum itself the same name. I also think it would be very helpful to have overloads of Raylib functions using the same names.

Perhaps the updater script can be designed in such a way as to leave in anything extra that's added to package.d, while replacing the parts where bindings to raylib symbols are declared or defined.

This can be made to work either in sections, where certain sections of package.d are dedicated to automatically-generated bindings, while other parts are for things that are added in manually, or it can be more detailed in deciding whether a certain line should be overwritten when doing the update.

A third option is we split package.d into multiple files (like I started doing in my port-functions branch), it may be rather simple to do this, as the end of each file after the extern(C) line can be dedicated to the automatically-generated functions, while the parts above will be left intact by the updater script.

I haven't managed to get comfortable with any of the D parsing libraries, so I would probably just write my own logic in the script to determine what's to be removed and what shouldn't on update, which of course may make it somewhat sensitive to formatting.

We can't remove existing symbols (unless raylib removes them). We can't change the meaning of them. At its heart, this is a binding with some D niceties added onto it. It should be usable as a binding first. Someone who has code that says

import raylib;

KeyboardKey k = KeyboardKey.KEY_A;

it should continue to compile.

I very much agree with this. Perhaps I should write a unittest to make sure all symbols are there, so that we know if the updater script removes anything. Maybe there can even be a test program with visual results to test if it's doing what it should. I can also go over the Raylib-D examples repository to test all the examples.

If you can produce a tool that is effective at generating these separate modules, then I'm fine adding it, as long as the core binding isn't affected. raylib-d can grow new features and niceties, but it must retain the existing features and API.

Alright. Would you rather package.d be split into multiple modules based on Raylib source files (like I started doing in my port-functions branch), or keep it all inside package.d?

@schveiguy
Copy link
Owner

Are you against putting anything that's not strictly a binding to a raylib symbol inside package.d?

Yes. But I'm not against package.d importing non-binding files (see for example https://github.com/schveiguy/raylib-d/blob/master/source/raylib/package.d#L87). But a module should be the exact translation of raylib.h, with basically nothing else in it.

What I don't want is to have to run the binding generator and then have to also write complex code in the same file. D has modules, we should use them.

Unfortunately, this would really limit us, as D requires that enums are defined in only one place, and all overloads of a function are defined in the same module.

I am not clear on the issue here. What are you trying to do with enums? Overloads of functions can cross module boundaries.

In response to your other points, I think we already have package.d split into other modules. package.d imports everything, and you can add more modules to that list if you want.

In terms of rewriting raylib functions in D, I am not interested for this project. That is my other project, draylib. Overloading to allow for proper strings is fine, but should be in a separate module, like raylib_d.d or something, that you can optionally import.

The goals here are: a simple binding, with basic niceties added on. I'm not looking for a raylib-cpp style API reimagining. At least for this project. For draylib, yes, I want to make everything D-friendly.

@LiamM32 LiamM32 marked this pull request as draft April 13, 2024 06:44
@LiamM32
Copy link
Contributor Author

LiamM32 commented Apr 13, 2024

Are you against putting anything that's not strictly a binding to a raylib symbol inside package.d?

Alright then. I'll try to put new things in other modules. The latest commit I did in my new-updater branch has the start of a script that's supposed to make the necessary edits to package.d while leaving in anything that was added, but I can change course if you want.

What do you think of the idea of adding default arguments to functions (such as setting the default colour to Colors.WHITE in the texture-drawing functions)? Do you consider this too much of a new thing to put in package.d?

I am not clear on the issue here. What are you trying to do with enums? Overloads of functions can cross module boundaries.
Exactly what I've already done with enums in my master branch; have both the existing extra-verbose enum members (MouseButton.MOUSE_BUTTON_LEFT) and some new shorter ones that are equivalent (MouseButton.LEFT). As far as I can tell, there is no way to partially define an enum in two separate modules.

Overloads of functions can cross module boundaries.
Can they? I got an error when I tried doing that, and I figured that it's because I'm not supposed to do that. If this isn't true, than that makes things easier.

In response to your other points, I think we already have package.d split into other modules. package.d imports everything, and you can add more modules to that list if you want.

Yay! I suppose that means they should just be deleted from package.d every time you do the automatic update?

Labelled some original comments in `source/raygui.d` as "D Note" so we know they weren't copied from upstream RayGUI.
`enumAlias` template replaces `enumMixin` CTFE function in `raygui` module.
Fixed the newest RayGUI icons, which were previously blank.
Added more functions to `Rectangle`.
Added "raylib" lib requirement to `dub.json`.
Set `raylib_types` modules to `@safe`, and added more rectangle functions.
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.

Make better enums
3 participants