forked from chocolate-doom/chocolate-doom
-
Notifications
You must be signed in to change notification settings - Fork 132
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
TrueColor: implement optional tablified translucency #1233
Draft
JNechaevsky
wants to merge
12
commits into
master
Choose a base branch
from
dirtyblend
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
7ba4db1
Working functions with 512 LUT translucency
JNechaevsky 6baa54e
Add config variable and support for Doom
JNechaevsky ae0ae84
Call pointer initialization function as well
JNechaevsky 609b1ff
Always use macro values for blending alphas
JNechaevsky 0873653
Implement per-game tables
JNechaevsky e4ba9d7
Braces and Crispy code style
JNechaevsky a843149
Rename to `crispy_truecolorblend`
JNechaevsky c41667b
Fix typo
JNechaevsky 183b0e4
Use two LUTs, not three
JNechaevsky c21e53a
Fair Play and few extra comments
JNechaevsky c1a3917
Ingame switch for Doom
JNechaevsky 92414e8
Ingame switch for Heretic and Hexen
JNechaevsky File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good already, thank you! Do you think we could get away with only two arrays, a "main" one and an "alt" one (which would be additive in Doom)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was thinking how to divide tablifying per game, but in the end chickened out because of:
if/else
conditions.for
loop.But looks likes it's not that scary, so sure, let's go this way. Most important question for this PR: how should we name menu item? Maybe something like:
Hot-switch is fairly simple, we just have to kick
R_InitBlendQuality
, even post-rendering doesn't seems to be not needed.Finally, I have an itch that something else could be done. Current implementation helps to improve performance and get higher average framerate in
-timedemo
runs, but results are not as high as expected. I'm thinking to play around withR_DrawTLColumn
by unrolling by four, @rfomin recommending to go farther by turning blending functions toinline
's, but it's tricky since we are using function pointers, except for BlendDark for fuzz drawing.So maybe you can give few recommendations regarding where to think or dig? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd be 9bit, strictly speaking. 😉
Translucency mode: speed / quality
I'd avoid using the term "blending" if we only call it "translucency" just one line above.
Sure, there's so much left for optimization in Doom's source code.
Nope, we cannot inline function pointers, obviously.
I'm fine with the current implementation. Heck, I was even too lazy for the last ~10 years to write the code that you just contributed in this PR. 😁
If you find some code path that could use some further optimization, sure, please feel free to investigate it. But this is probably topic for another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making more functions (e.g.
R_DrawTLColumnAdd
,R_DrawTLColumnOver
). Function calls on every pixel are expensive, I checked when implementing Woof's "function factory" (about ~30% performance hit).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dammit... There is just a small difference in performance comparing between 512x512 and 256x256 LUTs. Something really have to be done with column drawing functions.
Just in case, 256x256 LUT code, still much better than just 256 colors:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array access time doesn't depend on array size. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks, good to know - that's important. I'm playing around with
inline
-ing, at least for additive blending inR_DrawTLColumn
and it actually helps,-timedemo
differences before and after inlining are about ~466 w/o inlining and ~522 with inlining.Looks likes it is have to be done, maybe really in another PR, since we have to divide
R_DrawTLColumn
to blend/alt functions (and to high/low detail + high/low quality, oh God!), and unlikely I will handle Format Factory approach. Otherwise, this whole idea and implementation is nothing more than cosmetical change with tiny performance improvement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, tongue was ahead of thoughts. So the idea is put TrueColor TL drawing functions as macroses to
i_truecolor.h
where we can have inlining without overbloating existing code inr_draw.c
of all four games and we don't need to care abouttranmap[]
,tinttab[]
andxlatab[]
there. This will still require small separating ofcolfunc
pointers for TrueColor only, but in theory, this will do the trick.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't overload this PR, though. Let's do one step after the other...