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

CONFIGURE: Also install shaders if gles2 enabled #2513

Merged
merged 2 commits into from Oct 12, 2020

Conversation

@lmerckx
Copy link
Contributor

@lmerckx lmerckx commented Oct 12, 2020

Since the commit "CONFIGURE: Do not enable opengl_shaders feature flag for gles2. " (b3ffd89), the shaders are no more installed for opengles2 systems.
But they need them as they are used in the code.

I don't know if my way to test "USE_OPENGL_SHADERS or USE_GLES2" in Makefile is the standard of ScummVM but it works.
I'm open to any suggestions ...

@aquadran
Copy link
Member

@aquadran aquadran commented Oct 12, 2020

you are right, shaders should be installed on 'OR" conditions now. I'm not good enough with makefile syntax to comment :)
But in gerenal 👍

@@ -442,7 +442,7 @@ endif

# Shaders
DIST_FILES_SHADERS=
ifdef USE_OPENGL_SHADERS
ifneq ($(USE_OPENGL_SHADERS)$(USE_GLES2),)

This comment has been minimized.

@tsoliman

tsoliman Oct 12, 2020
Member

Interesting. I didn't know this but apparently it is the way to do conditional OR according to the GNU make docs

The docs also show usage of strip to remove white space, as otherwise whitespace values are interpreted as non-empty. Not sure if it is needed here.

I'm not great with make and so the intention wasn't obvious to me at all. Perhaps a comment might be helpful?

This comment has been minimized.

@lmerckx

lmerckx Oct 12, 2020
Author Contributor

Thanks for the proposition.
I've just added a comment.

@tsoliman
Copy link
Member

@tsoliman tsoliman commented Oct 12, 2020

Looking at this more, wouldn't this make more sense to be handled in configure?

Sort of keeping USE_OPENGL_SHADERS as the condition in the makefile but setting _opengl_shaders to yes if it was auto if _opengles2 is set to yes?

Basically revert the changes in the Makefile.common and ports.mk but make the changes in configure instead?

something like this:

if test "$_opengl_shaders" = auto && test "$_opengles2" = yes; then
        _opengl_shaders=yes
fi

This is a lot more clear and follows the intention better.

(Note: This will obviously break if someone configures with --enable-opengles2 and --disable-opengl-shaders but I think that's ok and a good thing. It's certainly better than silently enabling shaders when explicitly requesting disabling shaders)

@lmerckx
Copy link
Contributor Author

@lmerckx lmerckx commented Oct 12, 2020

It is almost what was did before b3ffd89:
When opengles2 was detected by the configure, opengl_shaders was also enabled.

But perhaps it implied non wanted side effects ?

@aquadran
Copy link
Member

@aquadran aquadran commented Oct 12, 2020

opengles2 and opengl_shaders are similar but needs to be separated, opengl_shaders stands for opengl (non-gles) but capable shaders. generally they cover most same code, but you can exclude each other if needed.

@aquadran aquadran merged commit 7bec7b7 into scummvm:master Oct 12, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
@aquadran
Copy link
Member

@aquadran aquadran commented Oct 12, 2020

I'm pushing this to unbreak builds. if needed we can continue discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.