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

Disable dynamic shadow dropdown on OGLES2 #12637

Merged
merged 7 commits into from Aug 4, 2022

Conversation

rollerozxa
Copy link
Member

As dynamic shadows will not be functional on OGLES2 for 5.6.0, the relevant dropdown should be hidden in the settings to prevent someone accidentally enabling it.

To do

This PR is Ready for Review.

How to test

Check so the dynamic shadow dropdown doesn't show up on OGLES2.

@sfan5 sfan5 added the Bugfix 🐛 PRs that fix a bug label Aug 2, 2022
@sfan5 sfan5 added this to the 5.6.0 milestone Aug 2, 2022
@JayAsPro
Copy link

JayAsPro commented Aug 2, 2022

Hello! Sorry for the question.
Does this mean that it will be impossible to use dynamic shadows on Android? If so, why? There is some technical limitation or obstacle? I really missed it while following the development.
Thanks!

@Extex101
Copy link

Extex101 commented Aug 3, 2022

Hello! Sorry for the question. Does this mean that it will be impossible to use dynamic shadows on Android? If so, why? There is some technical limitation or obstacle? I really missed it while following the development. Thanks!

The shaders would have to be completely rewritten for Ogles2.
So this change doesn't do much but prevent bugs/crash by removing the option to enable it when Ogles2 is enabled

@rollerozxa
Copy link
Member Author

Hello! Sorry for the question. Does this mean that it will be impossible to use dynamic shadows on Android? If so, why? There is some technical limitation or obstacle? I really missed it while following the development. Thanks!

Yes, the current dynamic shadows implementation does not work on OGLES2, see #11339.

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 3, 2022
@sfan5
Copy link
Member

sfan5 commented Aug 3, 2022

If I check the shader checkbox this happens and the mainmenu never works afterwards:

2022-08-03 21:27:14: ERROR[Main]: Main menu error: Runtime error from mod '' in callback handleMainMenuButtons(): minetest/bin/../builtin/mainmenu/tab_settings.lua:232: bad argument #2 to 'set' (string expected, got boolean)
2022-08-03 21:27:14: ERROR[Main]: stack traceback:
2022-08-03 21:27:14: ERROR[Main]: 	[C]: in function 'set'
2022-08-03 21:27:14: ERROR[Main]: 	minetest/bin/../builtin/mainmenu/tab_settings.lua:232: in function 'get_formspec'
2022-08-03 21:27:14: ERROR[Main]: 	minetest/bin/../builtin/fstk/tabview.lua:66: in function 'get_formspec'
2022-08-03 21:27:14: ERROR[Main]: 	minetest/bin/../builtin/fstk/ui.lua:101: in function 'update'
2022-08-03 21:27:14: ERROR[Main]: 	minetest/bin/../builtin/fstk/ui.lua:145: in function 'handle_buttons'
2022-08-03 21:27:14: ERROR[Main]: 	minetest/bin/../builtin/fstk/ui.lua:186: in function <minetest/bin/../builtin/fstk/ui.lua:172>

@rollerozxa
Copy link
Member Author

looks like I forgot to use set_bool, I'll fix that real quick

@sfan5
Copy link
Member

sfan5 commented Aug 3, 2022

You're also setting the boolean everytime the formspec is generated, that seems suboptimal.

@rollerozxa
Copy link
Member Author

Yeah I personally don't think the setting should be explicitly set to disabled like that, but it was a suggestion from Krock yesterday to prevent someone from accidentally enabling it.

@sfan5
Copy link
Member

sfan5 commented Aug 3, 2022

If at all that's the job of C++ imo.

Edit: another one
<+x2048> In #12637 Greyed out "Dynamic shadows" will pop up if shaders are disabled and disappear if they are enabled.

@rollerozxa
Copy link
Member Author

Yeah so adding && g_settings->get("video_driver") == "opengl" to every check for the enable_dynamic_shadows setting in C++?

@rollerozxa
Copy link
Member Author

Done. Please test this, as I haven't tested the C++ changes (still compiling)

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

@sfan5 sfan5 added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Aug 3, 2022
@sfan5 sfan5 merged commit eb49b6d into minetest:master Aug 4, 2022
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.

None yet

6 participants