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

Return to the main menu if a shader compilation fails #14256

Merged
merged 6 commits into from Jan 19, 2024

Conversation

HybridDog
Copy link
Contributor

Before this change, if the shaders are broken, only an error message is shown and the player enters the world nonetheless, where he/she sees broken graphics. For a shader developer, being thrown back to the main menu can be more convenient.

To throw the player back to the main menu, we add an exception. Since BaseException is only catched if NDEBUG is set, we add a new exception dedicated to shaders.

Closes: #10097
Previous pull request: #10240

To do

This PR is a Ready for Review.

How to test

  • Execute this in a Bash Shell:
    <<< nyan cat >> client/shaders/nodes_shader/opengl_fragment.glsl
  • Enter the game

sfan5
sfan5 previously approved these changes Jan 14, 2024
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

tested and works

src/client/shader.cpp Outdated Show resolved Hide resolved
src/client/clientlauncher.cpp Outdated Show resolved Hide resolved
src/client/shader.cpp Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 14, 2024
@grorp
Copy link
Member

grorp commented Jan 14, 2024

Also: For a better developer experience, the error message shown in the GUI should contain the actual error message from Irrlicht, not just a generic "shader compilation failed".

HybridDog and others added 3 commits January 15, 2024 18:56
Before this change, if the shaders are broken, only an error message is shown and the player enters the world nonetheless, where he/she sees broken graphics.
For a shader developer, being thrown back to the main menu can be more convenient.

To throw the player back to the main menu, we add an exception.
Since BaseException is only catched if NDEBUG is set, we add a new exception dedicated to shaders.
Co-authored-by: grorp <gregor.parzefall@posteo.de>
@HybridDog
Copy link
Contributor Author

How can I capture Irrlicht's error message, which arrives at MyEventReceiver::OnEvent?

@grorp
Copy link
Member

grorp commented Jan 15, 2024

How can I capture Irrlicht's error message, which arrives at MyEventReceiver::OnEvent?

64df516

What about this?

@sfan5
Copy link
Member

sfan5 commented Jan 15, 2024

Capturing the log messages from under the log subsystem only to put them into your exception text smells like a giant hack.

@grorp
Copy link
Member

grorp commented Jan 15, 2024

Capturing the log messages from under the log subsystem only to put them into your exception text smells like a giant hack.

It is, but less hacky approaches would require Irrlicht changes.

@HybridDog
Copy link
Contributor Author

64df516

What about this?

I have moved the log capturing into the class but now it's a lot of code.

@sfan5
Copy link
Member

sfan5 commented Jan 16, 2024

It is, but less hacky approaches would require Irrlicht changes.

The less hacky approach is to not do this.

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

I approve this PR both with and without 87c6497. However, I wonder about the amount of list-initialization in 87c6497.

The less hacky approach is to not do this.

I think getting a useful error message in the GUI is important. But I understand if you think it's not worth the hackyness.

src/client/shader.cpp Outdated Show resolved Hide resolved
src/client/shader.cpp Outdated Show resolved Hide resolved
src/client/shader.cpp Outdated Show resolved Hide resolved
@HybridDog
Copy link
Contributor Author

I have reverted the Irrlicht message capturing change for simplicity.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works. LGTM. With and without the log capturing mechanism. It could still be added later for convenience.

@SmallJoker SmallJoker added >= Two approvals ✅ ✅ and removed One approval ✅ ◻️ Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Jan 17, 2024
@sfan5 sfan5 merged commit f08e4bb into minetest:master Jan 19, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not join the game with broken shaders
6 participants