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

[misc] Rename Is*Ready() to Is*Valid() #3930

Open
raysan5 opened this issue Apr 22, 2024 · 34 comments · May be fixed by #3943
Open

[misc] Rename Is*Ready() to Is*Valid() #3930

raysan5 opened this issue Apr 22, 2024 · 34 comments · May be fixed by #3943

Comments

@raysan5
Copy link
Owner

raysan5 commented Apr 22, 2024

Potential functions to be reviewed:

bool IsWindowReady(void);                           // Check if window has been initialized successfully
bool IsShaderReady(Shader shader);                  // Check if a shader is ready
bool IsImageReady(Image image);                     // Check if an image is ready
bool IsTextureReady(Texture2D texture);             // Check if a texture is ready
bool IsRenderTextureReady(RenderTexture2D target);  // Check if a render texture is ready
bool IsFontReady(Font font);                        // Check if a font is ready
bool IsModelReady(Model model);                     // Check if a model is ready
bool IsMaterialReady(Material material);            // Check if a material is ready
bool IsAudioDeviceReady(void);                      // Check if audio device has been initialized successfully
bool IsWaveReady(Wave wave);                        // Checks if wave data is ready
bool IsSoundReady(Sound sound);                     // Checks if a sound is ready
bool IsMusicReady(Music music);                     // Checks if a music stream is ready
bool IsAudioStreamReady(AudioStream stream);        // Checks if an audio stream is ready
@RobLoach
Copy link
Contributor

That makes sense. I had originally named them Is*Ready() because I thought it matched a similar pattern in the old IsAudioDeviceReady().

@RenzoRomeo RenzoRomeo linked a pull request Apr 28, 2024 that will close this issue
@raysan5
Copy link
Owner Author

raysan5 commented Apr 29, 2024

@RobLoach Actually I think it makes sense for IsWindowReady() and IsAudioDeviceReady() (@RenzoRomeo) but I still have doubts if it should be reviewed for all the other data structures. In any case, it's a breaking change and I'm considering it carefully...

@ColleagueRiley
Copy link
Contributor

Why change something inconsequential that breaks compatibility rather than adding prefixes?

@krisvers
Copy link

krisvers commented Apr 29, 2024

With what Riley said, adding prefixes is much more important of a compatibility break than renaming Ready to Valid as there are so many namespace collisions with winapi. Although, I think that if one is to be considered, then the other should be as equally important.

@raysan5
Copy link
Owner Author

raysan5 commented Apr 29, 2024

@krisvers Sorry, no plans to add prefixes to raylib functions.

@krisvers
Copy link

krisvers commented Apr 29, 2024

@raysan5 Why not? There are many problematic collisions with winapi, and no prefix can cause unnecessary future namespace collisions.

@RobLoach
Copy link
Contributor

@krisvers That's a separate issue. For a discussion about it, see #1217 .

@krisvers
Copy link

I understand that, but if a compatibility change for functions is being considered then it makes sense to also consider other sensible function name changes like using a prefix to prevent namespace collisions.

@raysan5
Copy link
Owner Author

raysan5 commented Apr 29, 2024

@krisvers Simbol collisions can already be avoided. I can consider slightly renaming some functions added 6 months ago but not the full library. If anyone really has that need can fork the project.

@orcmid
Copy link
Contributor

orcmid commented Apr 29, 2024

@krisvers

if a compatibility change for functions is being considered then it makes sense to also consider other sensible function name changes like using a prefix to prevent namespace collisions.
This argument completely disregards all of the code in existing raylib apps out there that will suddenly not compile with such a gigantic breaking change, forcing untold amounts of work. That's not even concerning what has to change on the raylib wiki and other documentation. Whatever the @raysan5 rational for non-prefixing is, I support the conclusion.

@orcmid
Copy link
Contributor

orcmid commented Apr 29, 2024

@raysan5 @krisvers

@krisvers Simbol collisions can already be avoided. I can consider slightly renaming some functions added 6 months ago but not the full library. If anyone really has that need can fork the project.

Unless there is some hideous embedding of raylib inside of a considerable Windows app, my (limited) experience is that dependencies on platform functions that require windows.h can be isolated in safely named functions compiled from separate source files that #include windows.h and are statically linked with the raylib app. That turns out to be a clean way of platform-dependency isolation.

@krisvers
Copy link

Yeah it is fair to consider that the addition of a prefix would break almost all backwards compatibility. However, a prefix for this project should have been from day one especially with many function names being the same as in other mainstream libraries like the winapi. You can't change the past though 😔

@raysan5
Copy link
Owner Author

raysan5 commented Apr 29, 2024

many function names being the same as in other mainstream libraries like the winapi

@krisvers This is not true. Afaik, only 2 function names could potentially collide and it can be avoided.

@ColleagueRiley
Copy link
Contributor

many function names being the same as in other mainstream libraries like the winapi

@krisvers This is not true. Afaik, only 2 function names could potentially collide and it can be avoided.

Iirc, it was more than two, also including starts like Rectangle which share function names.

@raysan5
Copy link
Owner Author

raysan5 commented Apr 29, 2024

Anyway, this is a pointless discussion, this is my final message on the topic.

If you have some complaint, probably better direct them to Microsoft and Winapi than raylib.

@krisvers
Copy link

Anyway, this is a pointless discussion, this is my final message on the topic.

If you have some complaint, probably better direct them to Microsoft and Winapi than raylib.

What? Raylib colliding with Winapi is not a problem with Winapi, but instead Raylib lol. Though there are annoying things like near, far covered by defining the lean and mean macro. Considering how you are against breaking compatibility, what do you think would happen if Winapi changed to suit a C game library. This isn't exactly a pointless discussion but I will yield.

@RobLoach
Copy link
Contributor

Stop diluting this issue. I've created a discussion for windows.h conflicts at #3945 . Feel free to discuss naming conflicts there.

@orcmid
Copy link
Contributor

orcmid commented Apr 30, 2024

@krisvers

However, a prefix for this project should have been from day one especially with many function names being the same as in other mainstream libraries like the winapi. You can't change the past though 😔

For the sake of historical tidiness, it is valuable to know that BGI, the Borland Graphical Interface inspiration for raylib was used on MS-DOS forms prior to Windows.

No matter. Platforms will continue to change and require adapter layers of one kind and another while keeping the essential API of raylib stable. So long as the implementation is in ISO Standard C, the handling of libraries and other dependencies will be anchored accordingly.

@luis605
Copy link
Contributor

luis605 commented May 5, 2024

Potential functions to be reviewed:

bool IsWindowReady(void);                           // Check if window has been initialized successfully
bool IsShaderReady(Shader shader);                  // Check if a shader is ready
bool IsImageReady(Image image);                     // Check if an image is ready
bool IsTextureReady(Texture2D texture);             // Check if a texture is ready
bool IsRenderTextureReady(RenderTexture2D target);  // Check if a render texture is ready
bool IsFontReady(Font font);                        // Check if a font is ready
bool IsModelReady(Model model);                     // Check if a model is ready
bool IsMaterialReady(Material material);            // Check if a material is ready
bool IsAudioDeviceReady(void);                      // Check if audio device has been initialized successfully
bool IsWaveReady(Wave wave);                        // Checks if wave data is ready
bool IsSoundReady(Sound sound);                     // Checks if a sound is ready
bool IsMusicReady(Music music);                     // Checks if a music stream is ready
bool IsAudioStreamReady(AudioStream stream);        // Checks if an audio stream is ready

This is a good idea to make the API more intuitive, however it would break compatibility with many existing projects. I just don't see the point of breaking thousands of projects with these renames. I believe the function/enums naming should be well considered before they are pushed into master. It seems to late to fix it now. The only solution I see is to keep the old functions and add new ones that call the old ones:

void newFunctionName() {
    oldFunctionName();
}

void oldFunctionName() {
    // DO STUFF
}

This way there wouldn't be the need to break already existing projects, while improving the API clarity. However just like Ray said on Discord, [that's]

too much hassle

@raysan5
Copy link
Owner Author

raysan5 commented May 5, 2024

@luis605 The functions mentioned in this issue were added on latest release, a few months ago and they don't seem to be key functions that many users could use. I think we are still on time for a quick rename with not much pain involved. Renaming all Image*(), Text*(), Mesh*(), Wave*()... functions it's a completely different issue and yes, it's too much hassle.

@luis605
Copy link
Contributor

luis605 commented May 5, 2024

@raysan5 Then I suppose you should rename the functions for better clarity, because the API naming lacks some clarity in my opinion.

@raysan5
Copy link
Owner Author

raysan5 commented May 5, 2024

because the API naming lacks some clarity in my opinion.

Please, could you ellaborate why IsTextureReady() is so confusing in comparison to IsTextureValid()? Because, sincerely, I don't see that much difference...

@luis605
Copy link
Contributor

luis605 commented May 5, 2024

When you use "IsReady()" to check the status of an object, like "IsModelReady()", it implies that the object is fully prepared and operational for use. However, in some cases, the object might be valid (initialized) but not necessarily ready for usage. For instance, in the context of a model, all pointers might be initialized (valid) but not fully ready for usage (ex. if the vertices count is different than the actual number of vertices in the model).

Therefore, renaming the function to "IsValid()" would better reflect its purpose, indicating that it simply checks if the object is in a valid state, regardless of its readiness for use. This change in semantics would make the function's purpose clearer and more aligned with its actual behavior.

@RobLoach
Copy link
Contributor

RobLoach commented May 5, 2024

however it would break compatibility with many existing projects.

For backwards compatibility of function names in projects, defines are an option. While raylib may not prioritize backwards compatibility, it can still be accomplished in other means...

#define IsModelReady IsModelValid

@orcmid
Copy link
Contributor

orcmid commented May 5, 2024

@luis605
I agree that the name should reflect what is actually determined.

On the other hand, I am unclear what the user action is expected to be on !Is*Valid and how this could matter. It seems from the example about vertex counting that Is*Complete or Is*Prepared might be preferable terms, and I still am unclear what action an application can take in a releasable app.

I think it might also be important to be clear what the operational behavior will be when a *-operation is attempted when !Is*Valid.

@luis605
Copy link
Contributor

luis605 commented May 5, 2024

@orcmid Precisely, when an object is not valid, the best thing to do is not to use it and, if possible, reload it. Otherwise you might get into undefined behavior, segfaults, etc. which may crash the application or make it unusable. It would also be nice for this functions to have a logging mechanism to alert the developer/user what the problem is, so he/she can further troubleshoot.

@raysan5
Copy link
Owner Author

raysan5 commented May 5, 2024

@luis605 the logging mechanism has been there for many years, when a resource loading fails it is logged.

When you use "IsReady()" to check the status of an object, like "IsModelReady()", it implies that the object is fully prepared and operational for use.

Yes, and that's the case. We can split the loading-initialization-dataValidation in as many states as desired but it doesn't mean one syntax is better than another. What about an object that IsValid() but it's !IsReady() to use (i.e. Compressed Texture not supported by GPU but loaded successfully). What would happen in that case? Would be the syntax good enough for users?

@orcmid
Copy link
Contributor

orcmid commented May 5, 2024

@luis605 > It would also be nice for this functions to have a logging mechanism to alert the developer/user what the problem is, so he/she can further troubleshoot.

I can't imagine this being expected of and meaningful for a game's end-user. The closest I can think of is a AAA game reporting that something is wrong and it is closing, with a location of a log that can be used in messaging the game developer's support location. In advanced cases, submission of the log can be an option in the error dialog.

If it's a debug provision, or tied into an assert, I think the !Valid case should always be treated as fatal in a release's code. This does not seem to be part of the conceptual model for raylib. I don't think I have anything useful to offer about that.

@jamesl-github
Copy link

its ok to do breaking changes
sdl3 is renaming many functions that existed in sdl2
its not that difficult for a developer to do a global replace of the 10 different functions that might be affected
sure, more than 10 functions are being renamed -- but how many are actually used in a single project

so rename everything, fix and be consistent

@raysan5
Copy link
Owner Author

raysan5 commented May 5, 2024

@jamesl-github I agree, actually raylib implements (some) breaking changes in every minor release and I'm ok with that. Here the discussion is more focused on the correctness of those changes and syntax itself.

@orcmid
Copy link
Contributor

orcmid commented May 5, 2024

@jamesl-github @raylib
There were major breaking changes between Python 2 and Python 3, just as there will be for SDL3 versus SDL2. And in both cases, the earlier progressions persist for some time and continue to have updates. I haven't followed closely enough to know if Python 2 is deprecated. SDL2 is definitely alive and well. We also have the lessons of Windows major releases (even 11 versus 10) and overlapping support periods, as in end-user Linux distributions.

So, if we are talking about a raylib 6 here, certainly. A 5.1 with that many breaking changes and taking over the progression seems sketchy to me. It is understandable that there are no patch releases in raylib, and there is no backporting from -dev version. That makes breaking changes more difficult though, especially if that's also the only way to get important bug fixes.

@Fhoughton
Copy link

Fhoughton commented May 9, 2024

I think this is a difficult issue because the Is*Ready() functions are really validity checks, such as IsTextureReady() below, but when used I expect that they're primarily applied as readyness checks. I think the question is partly about their use case, and that whilst using Ready is closer to what people may primarily use them for I think Valid is a much more clear indicator of their actual use case and the depth of the checks they perform (such as checking that textures have a non-zero width and height). Overall I think that renaming them to Is*Valid() is the right action for most cases, as it better explains the function behaviour and prevents confusion.

raylib/src/rtextures.c

Lines 3963 to 3967 in 6ec9255

if ((texture.id > 0) && // Validate OpenGL id
(texture.width > 0) &&
(texture.height > 0) && // Validate texture size
(texture.format > 0) && // Validate texture pixel format
(texture.mipmaps > 0)) result = true; // Validate texture mipmaps (at least 1 for basic mipmap level)

Perhaps off-topic but if they are renamed I would support changing their definition to also return the reason for failure, though maybe that's out of scope and makes the API change more severe.

Edit:
There's also the option of moving the current implementations of the Is*Ready() functions to Is*Valid() and keeping the current names with implementations that check just for availability and not validity (e.g. just checking the texture has a valid OpenGL id) but that might just be overcomplicating the API to support a questionable use case.

@orcmid
Copy link
Contributor

orcmid commented May 9, 2024

@Fhoughton @raysan5
I am still having difficulty with "Valid." And if these really are failure cases, we need to know that trying again doesn't change anything. So I agree, "Ready" is not very indicative.

I think "Usable" is more pertinent. "Available" is too ambiguous.

@RobLoach
Copy link
Contributor

RobLoach commented May 9, 2024

I initially put these functions in place to allow checking the failure state of the Load() functions without having to go into the internals of the struct itself...

Shader shader = LoadShader(0, "reload.fs");

// Before
if (shader.loc == NULL) {
    // Failed to load shader
}

// After
if (!IsShaderReady(shader)) {
    // Failed to load shader
}

I named them IsReady() to align with the naming convention of the two other Ready functions at the time:

IsWindowReady()
IsAudioDeviceReady()

I don't think there is a need to complicate it more than is needed. The real goal of these functions was for an easy way to check against the fail-state of the load functions. IsLoaded() is another option. Keep It Stupid Simple.

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 a pull request may close this issue.

8 participants