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

GPU: Functions that may return errors should call SDL_SetError() #10779

Closed
Akaricchi opened this issue Sep 10, 2024 · 13 comments · Fixed by #10958
Closed

GPU: Functions that may return errors should call SDL_SetError() #10779

Akaricchi opened this issue Sep 10, 2024 · 13 comments · Fixed by #10958
Assignees
Milestone

Comments

@Akaricchi
Copy link
Contributor

Currently most (all?) of the error reporting is done through the SDL log, and APIs that may fail (e.g. SDL_CreateGPUDevice) do not call SDL_SetError(). This makes it impossible to, for example, display a message box with the failure reason.

@slouken
Copy link
Collaborator

slouken commented Sep 10, 2024

The log calls should generally be switched to SDL_SetError()

@slouken slouken added this to the 3.0 ABI milestone Sep 10, 2024
@thatcosmonaut
Copy link
Collaborator

Error setting is reasonable on things like creation functions, where we return NULL if the creation failed, and we should do that. Error setting on hot-path commands will just be futile - those functions shouldn't return anything but void and nobody can check them for errors without turning their client code into an absolute mess.

@slouken
Copy link
Collaborator

slouken commented Sep 10, 2024

We should probably have a similar policy with the SDL render API, and possibly add a debug layer there. @icculus, thoughts?

@icculus
Copy link
Collaborator

icculus commented Sep 10, 2024

I think the render layer is okay as-is: it uses SDL_SetError where reasonable and doesn't need a debug layer because there isn't really much you can do with it that's a surprising silent failure, unlike using OpenGL directly.

@thatcosmonaut
Copy link
Collaborator

We decided that anything where the underlying APIs return error codes is something we should bubble up into GPU, anything else is probably unrecoverable or we can catch obvious failures with debug mode validations.

@thatcosmonaut
Copy link
Collaborator

Resolved by 557c6df

@slouken
Copy link
Collaborator

slouken commented Sep 25, 2024

Not yet resolved, see comments.

@slouken slouken reopened this Sep 25, 2024
@thatcosmonaut
Copy link
Collaborator

Maybe actually resolved by 925e47a ?

@thatcosmonaut
Copy link
Collaborator

thatcosmonaut commented Sep 25, 2024

@slouken If it's OK to call both LogError and SetError I'll go ahead and just do that, but isn't it a bit redundant with SDL_LOG_PRIORITY_DEBUG?

@slouken
Copy link
Collaborator

slouken commented Sep 25, 2024

I'm thinking something like this:

SDL_SetError(...);
if (debug) {
    SDL_LogError(SDL_LOG_CATEGORY_GPU, ...);
}

Setting an error doesn't automatically log, and having an error in the GPU log category means that the application can filter it appropriately.

As an aside, we should have a hint that forces debug on so users can enable debug validation and error printing from the environment.

@thatcosmonaut
Copy link
Collaborator

That seems alright to me, but I do worry that not logging errors at all if debug mode is off might cause difficulties for clients if errors come up in deployment.

@slouken
Copy link
Collaborator

slouken commented Sep 25, 2024

That seems alright to me, but I do worry that not logging errors at all if debug mode is off might cause difficulties for clients if errors come up in deployment.

That's what the hint is for. "Just set this environment variable (or enable the "Debug GPU option") and send me the debug output"

@thatcosmonaut
Copy link
Collaborator

Alright, I'm going to revise the functions further tomorrow, we need to restructure some of them to cleanly return errors.

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.

5 participants