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

Minor OpenGL Dark Overlay Fix #557

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

andrikpowell
Copy link
Contributor

I tried to get everything ready in the last big PR, but apparently I missed something.

During the last PR, I found and fixed an issue where the dark overlay would actually become inverted when the Invulnerability effect was on.

However my fix accidently made it so that the overlay ignored screen effects (red pain screen, yellow item screen). So this PR allows those effects to show on the overlay, just as it does on Software.

Let me know if there's a better way to simplify this code, because it seems a bit messier than I'd like, but this is the only way I could get it to not invert either the automap or menu overlay.

@Pedro-Beirao
Copy link
Collaborator

Can you explain the problem further?
Without this PR, the red/yellow screens are showing up fine in overlay for me

@andrikpowell
Copy link
Contributor Author

Can you explain the problem further? Without this PR, the red/yellow screens are showing up fine in overlay for me

This is the easiest way I can explain the issue:
https://www.youtube.com/watch?v=BnnaNSVmR3k

@Pedro-Beirao
Copy link
Collaborator

Pedro-Beirao commented Dec 13, 2024

Alright, I understand now.
About the code, put the second parameter on the line below.

@andrikpowell
Copy link
Contributor Author

About the code, put the second parameter on the line below.

Should be good now 👍

Copy link
Collaborator

@rfomin rfomin left a comment

Choose a reason for hiding this comment

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

LGTM

@fabiangreffrath
Copy link
Collaborator

fabiangreffrath commented Jan 15, 2025

What I don't like here is the mixture of conditions, function return values and actual variables that are passed over to gld_LookupIndexedColor(). Maybe you could factor this out into one or two temporary variables, so it's more clear what happens in which case.

@andrikpowell
Copy link
Contributor Author

andrikpowell commented Jan 15, 2025

What I don't like here is the mixture of conditions, function return values and actual variables that are passed over to gld_LookupIndexedColor(). Maybe you could factor this out into one or two temporary variables, so it's more clear what happens in which case.

Would you be happier if we went with this instead:
int col = invul_cm && !V_IsAutomapLightmodeIndexed() ? playpal_white : playpal_black;
color_rgb_t color = gld_LookupIndexedColor(col, V_IsUILightmodeIndexed() || V_IsAutomapLightmodeIndexed());

Or even more broken down::
dboolean automap = V_IsAutomapLightmodeIndexed()
int col = invul_cm && !automap ? playpal_white : playpal_black;
color_rgb_t color = gld_LookupIndexedColor(col, V_IsUILightmodeIndexed() || V_IsAutomapLightmodeIndexed());

The problem here is that all these conditions are needed to fully fix this issue.

Regarding the V_IsUILightmodeIndexed() || V_IsAutomapLightmodeIndexed() condition, that is commonly used in the DSDA OpenGL code for the condition of gld_LookupIndexedColor(). Before this PR, I tried to simplify this condition, but that caused screen palettes to not apply to the overlay.

The invul_cm ? playpal_white : playpal_black is required to avoid the menu overlay turning white during invuln effect. The !V_IsAutomapLightmodeIndexed() was added to the invul_cm code to avoid the automap overlay becoming white during invuln.

@fabiangreffrath
Copy link
Collaborator

Honestly, I'd indeed prefer the second more broken down variant. The problem is not that all these conditions are needed, that's fine. The "problem" is that they are all mangled together and passed into the gld_LookupIndexedColor() function without any further context. And I'd even appreciate if you could add (parts of) the last two paragraphs in your reply as a comment.

@andrikpowell
Copy link
Contributor Author

Did a force push with new code with comments.

Copy link
Collaborator

@fabiangreffrath fabiangreffrath left a comment

Choose a reason for hiding this comment

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

Better now, thanks!

@fabiangreffrath fabiangreffrath merged commit 5742b09 into kraflab:master Jan 15, 2025
5 checks passed
@andrikpowell andrikpowell deleted the opengl-overlay-fix branch January 15, 2025 15:27
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 this pull request may close these issues.

4 participants