RGB666 color matcher
Moderator: GZDoom Developers
Re: RGB666 color matcher
This code is now on the QZDoom master.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49228
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: RGB666 color matcher
I guess that's the end of sharing code then. I am not going to add this.
Re: RGB666 color matcher
I am all for optimizations if they are available and I can try them. It's not my intent to block upstreams to ZDoom with this submission. If you have any ideas on how I can make this faster, I am open to trying them.
But I've already worked a very long time on this - too long to just abandon it - and also too long to just let it sit and let it create a shit ton of merge incompatibilities down the line - and the performance impact is minimal except in the most abusive maps - most typical maps will not see a performance difference at all, even ones that do use transparency. It really comes down to speed vs quality. I made a compromise by adding an option to use the old code, instead, and that version is what has been merged into QZDoom.
But no one wants to work from two separate code bases, especially when this many lines are involved. The ZDoom renderer as-is is not acceptable to work with, and the code clean-ups that QZDoom has already taken will not be the last. It's either stick with a flawed infrastructure and be held back - or move forward. If ZDoom doesn't want to upstream that - so be it.
But I've already worked a very long time on this - too long to just abandon it - and also too long to just let it sit and let it create a shit ton of merge incompatibilities down the line - and the performance impact is minimal except in the most abusive maps - most typical maps will not see a performance difference at all, even ones that do use transparency. It really comes down to speed vs quality. I made a compromise by adding an option to use the old code, instead, and that version is what has been merged into QZDoom.
But no one wants to work from two separate code bases, especially when this many lines are involved. The ZDoom renderer as-is is not acceptable to work with, and the code clean-ups that QZDoom has already taken will not be the last. It's either stick with a flawed infrastructure and be held back - or move forward. If ZDoom doesn't want to upstream that - so be it.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49228
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: RGB666 color matcher
I am currently toying around with it myself. The main problem is that all the bit masking madness for the 555 version is far to obtuse to be replicated. For example, what is fg2rgb and bg2rgb? Normally, doing a 666 mask shouldn't be any more complicated than doing a 555 mask.
Other things: The fill drawers could pull a lot of stuff out of the loop, they also multiply by _srcalpha twice. Intentional or not?
Other things: The fill drawers could pull a lot of stuff out of the loop, they also multiply by _srcalpha twice. Intentional or not?
Re: RGB666 color matcher
Premultipled swizzle tables for RGB values for the game palette. They point to a Col2RGB8 table entry, in which there are 64 levels in an array[], ranging from black to full color opacity, corresponding to every game palette entry. They are used in blend math, again, for premultiplied opacity to a palette entry on every channel. There's a decent bit of documentation about it in the v_* and r_* files. It is not possible to directly repeat the swizzle tables in rgb666 because it requires at least 6 masking fields, and with 6 bits per channel that's at least 36 bits that it needs, which is more than the 32-bits required in a DWORD (and no, I am not going to try this in a QWORD - that's going to absolutely kill 32-bit builds in performance).Graf Zahl wrote:For example, what is fg2rgb and bg2rgb?
Probably not intentional. If you want to make adjustments, feel free. The goal was to get an implementation to work with in the first place - tweaking it as required to fix problems or make it quicker should naturally be encouraged.Graf Zahl wrote:Other things: The fill drawers could pull a lot of stuff out of the loop, they also multiply by _srcalpha twice. Intentional or not?
- EDIT: Here's some documentation about how Col2RGB8 and ZDoom's premultiplied tables for RGB555 blending works: https://github.com/rheit/zdoom/blob/mas ... deo.h#L449 - I could've sworn I found some more, if I do, I'll post it.
Re: RGB666 color matcher
Alright. This is one last attempt at appealing this decision. After this - I am sorry, but QZDoom must move forward. We'd prefer not to leave the ZDoom code base behind, but that is the only choice we have at this point going forward - but this is, perhaps, a vain attempt to keep that from happening.
For this test, I've done the following:
I set "r_multithreaded false" - get some approximation of what it's like to run this on an old CPU.
I also went into my power settings, and crippled my CPU's speed down to 775 MHz. That's well below what was available 15 years ago.
As I stated, the compromise solution I had was to make both drawers available - and allow the user to switch between them. I was careful to put the conditional for them *outside* the loops, and as a result speed was not noticeably affected at all. (Honestly, you'd be amazed how *little* the drawers themselves are called, compared to how much they actually draw - this is why performance was not very much affected by the conditional statements)
So really - holding back the code base for ZDoom at this point has very little to do with the speed of the drawers themselves, now. Yet you accepted a code submission that had actual extreme performance degredation with barely even a word about it. Caution about performance drops is warranted - but your caution is clearly in the wrong place to have accepted that. (Which is why QZDoom has not picked up on this submission yet)
Additionally - the RGB555 drawers are now in use by *default* - which means that no one will even notice a change until they enable the rgb666 code in the menu.
If you want me to do something, or try something, let me know. But making blanket statements like "make this faster" will not do. I already tried, and I don't know of any solution besides the bit shifting which I really don't know if I want to reuse because of just how unclear it is to the average person (it took me about 3-4 hours to understand how it worked, and I didn't even know the v_video.h documentation was there). If I do, I am going to swizzle it back very much differently, and my algorithm for the re-swizzle will be quite different from ZDoom's and possibly even slower, as well. To put it quite bluntly: This is a clear speed vs precision issue. Is precision forbidden for those who want it, when speed is still available otherwise?
For this test, I've done the following:
I set "r_multithreaded false" - get some approximation of what it's like to run this on an old CPU.
I also went into my power settings, and crippled my CPU's speed down to 775 MHz. That's well below what was available 15 years ago.
Spoiler:I really believe that more practical tests are much more valuable than theoretical tests on the drawers directly - to see the impact that they have on the game at large. As you can see - there's very little difference even in places where translucent linedefs do get abused. The only real significant case of difference was dead.air - and I am not sure why the drawers are so drastically different there, but they are.
As I stated, the compromise solution I had was to make both drawers available - and allow the user to switch between them. I was careful to put the conditional for them *outside* the loops, and as a result speed was not noticeably affected at all. (Honestly, you'd be amazed how *little* the drawers themselves are called, compared to how much they actually draw - this is why performance was not very much affected by the conditional statements)
So really - holding back the code base for ZDoom at this point has very little to do with the speed of the drawers themselves, now. Yet you accepted a code submission that had actual extreme performance degredation with barely even a word about it. Caution about performance drops is warranted - but your caution is clearly in the wrong place to have accepted that. (Which is why QZDoom has not picked up on this submission yet)
Additionally - the RGB555 drawers are now in use by *default* - which means that no one will even notice a change until they enable the rgb666 code in the menu.
If you want me to do something, or try something, let me know. But making blanket statements like "make this faster" will not do. I already tried, and I don't know of any solution besides the bit shifting which I really don't know if I want to reuse because of just how unclear it is to the average person (it took me about 3-4 hours to understand how it worked, and I didn't even know the v_video.h documentation was there). If I do, I am going to swizzle it back very much differently, and my algorithm for the re-swizzle will be quite different from ZDoom's and possibly even slower, as well. To put it quite bluntly: This is a clear speed vs precision issue. Is precision forbidden for those who want it, when speed is still available otherwise?
Last edited by Rachael on Mon Dec 26, 2016 1:40 am, edited 4 times in total.
-
Major Cooke
- Posts: 8209
- Joined: Sun Jan 28, 2007 3:55 pm
- Preferred Pronouns: He/Him
- Operating System Version (Optional): Windows 10
- Graphics Processor: nVidia with Vulkan support
- Location: GZBoomer Town
- Contact:
Re: RGB666 color matcher
It's true, that one is causing massive issues with frame rates. I had to revert that one myself (ZZYZX's thing)
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49228
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: RGB666 color matcher
Graf Zahl wrote: Other things: The fill drawers could pull a lot of stuff out of the loop, they also multiply by _srcalpha twice. Intentional or not?
This won't get merged until the above gets addressed. It just looks wrong to me.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49228
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: RGB666 color matcher
I just updated QZDoom and saw that dpJudas was removing all the r_columnmethod and rt drawer stuff, including a major part of this PR.
I'm sorry, but if you want a bit more coordination with ZDoom, you have to change how you do things. It cannot be that you dump this mess on me and at the same time dpJudas is removing the vast majority of it. As things look now, this PR is a major waste of effort and I will not bother any longer with it.
The main reason why I kept GZDoom sane is that I deliberately avoided getting into situations where the code base started to deviate too much - often resulting in preemptive changes to ZDoom to allow easier handling before implementing the new feature. I see no such effort here, you happily code away and maybe afterward think about the implications. If you want to continue on this route, feel free to do, but then please do not bother me with it. But I strongly recommend being a bit more forthcoming with such stuff and coordinate its potential integration into ZDoom. That also means discussing such changes. I only found out about the drawers because I wanted to have a look at the dynamic light code.
If you can put together a PR that includes both dpJudas's cleanup of the drawer stuff - which indeed is a waste of code and effort to maintain - and the remains of this, I'll gladly accept it, but as things look now, adding this makes no sense.
I'm sorry, but if you want a bit more coordination with ZDoom, you have to change how you do things. It cannot be that you dump this mess on me and at the same time dpJudas is removing the vast majority of it. As things look now, this PR is a major waste of effort and I will not bother any longer with it.
The main reason why I kept GZDoom sane is that I deliberately avoided getting into situations where the code base started to deviate too much - often resulting in preemptive changes to ZDoom to allow easier handling before implementing the new feature. I see no such effort here, you happily code away and maybe afterward think about the implications. If you want to continue on this route, feel free to do, but then please do not bother me with it. But I strongly recommend being a bit more forthcoming with such stuff and coordinate its potential integration into ZDoom. That also means discussing such changes. I only found out about the drawers because I wanted to have a look at the dynamic light code.
If you can put together a PR that includes both dpJudas's cleanup of the drawer stuff - which indeed is a waste of code and effort to maintain - and the remains of this, I'll gladly accept it, but as things look now, adding this makes no sense.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49228
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: RGB666 color matcher
And one more thing: When offering both methods as an option, make sure that it's worth the code duplication. For the screen wipes that clearly is not the case, as they are locked to a 35 fps update rate, so if they are a bit slow, nobody will notice.
Re: RGB666 color matcher
Fixed.Graf Zahl wrote:Graf Zahl wrote: Other things: The fill drawers could pull a lot of stuff out of the loop, they also multiply by _srcalpha twice. Intentional or not?
This won't get merged until the above gets addressed. It just looks wrong to me.
Re: RGB666 color matcher
I was going to address that after the issues in this PR got resolved. But that's okay - no biggy. I am done with this.Graf Zahl wrote:I just updated QZDoom and saw that dpJudas was removing all the r_columnmethod and rt drawer stuff, including a major part of this PR.
I'm sorry, but if you want a bit more coordination with ZDoom, you have to change how you do things. It cannot be that you dump this mess on me and at the same time dpJudas is removing the vast majority of it. As things look now, this PR is a major waste of effort and I will not bother any longer with it.
The main reason why I kept GZDoom sane is that I deliberately avoided getting into situations where the code base started to deviate too much - often resulting in preemptive changes to ZDoom to allow easier handling before implementing the new feature. I see no such effort here, you happily code away and maybe afterward think about the implications. If you want to continue on this route, feel free to do, but then please do not bother me with it. But I strongly recommend being a bit more forthcoming with such stuff and coordinate its potential integration into ZDoom. That also means discussing such changes. I only found out about the drawers because I wanted to have a look at the dynamic light code.
If you can put together a PR that includes both dpJudas's cleanup of the drawer stuff - which indeed is a waste of code and effort to maintain - and the remains of this, I'll gladly accept it, but as things look now, adding this makes no sense.
-
Hell Theatre
- Posts: 23
- Joined: Sat Apr 30, 2016 10:33 am
Re: RGB666 color matcher
Is this really that important to cause a potential rift?
I just checked out this code and I have to admit, I hardly see any benefit for translucent blending. I can see how using more color information was of benefit for remapping textures but here the results are far less thrilling. The cost-to-benefit ratio is simply not convincing and certainly not worth adding this much new code - and certainly not worth driving a wedge between both ports.
With most of the stuff that came from QZDoom so far it was mostly apparent what the point was, but this one utterly fails to convince me.
I just checked out this code and I have to admit, I hardly see any benefit for translucent blending. I can see how using more color information was of benefit for remapping textures but here the results are far less thrilling. The cost-to-benefit ratio is simply not convincing and certainly not worth adding this much new code - and certainly not worth driving a wedge between both ports.
With most of the stuff that came from QZDoom so far it was mostly apparent what the point was, but this one utterly fails to convince me.
Re: RGB666 color matcher
It was important for me to say I am no longer working on the old drawers. That's it. Those drawers are way too much work. Whatever rifts that may cause - not my problem.Hell Theatre wrote:Is this really that important to cause a potential rift?
Graf knows where the QZDoom code base is, and clearly knows how to cherry-pick. If he wants code, he's more than welcome to it.
But I am done submitting PR's for ZDoom - with this kind of scrutiny when someone else's submission which causes even more problems is accepted with far less complaints from him - I'm not going to put forth the extra effort to bother.
Besides, he doesn't want QZDoom's code in its current state, anyhow. We haven't finished the voxels.
-
Hell Theatre
- Posts: 23
- Joined: Sat Apr 30, 2016 10:33 am
Re: RGB666 color matcher
Sigh...
That's really missing the forest for the trees.
That 'other' submission may have had an unexpected problem - one, I might add, I haven't been able to reproduce myself, and apparently Graf hasn't either. And whatever the cause is, rest assured it will be fixed, but if you look at the submission - or rather what's left of it - is some superficial changes to a handful of functions, while yours completely messes up two large source files.
But one word about QZDoom:
I was a bit surprised by Graf's post that prompted this, but looking at it he seems to be right: If you want to experiment, feel free to experiment, but just looking at your repo, you are making it very hard to get stuff out of there, it's all an incoherent mix of experimentation, which should be local to your port, cleanup, which should be backported ASAP and a coding style that's not conductive to collaboration.
Just taking the RGB666 stuff is a textbook example of doing it wrong: Instead of keeping the code separate from the RGB555 code and making the selection somewhere where code consistency is less important, like the place where the drawers are being called and splitting off into two separate drawers there, you changed every single one of them separately. Your entire work basically forced Graf's hand, he'd either go along or leave you in the dust. Seems that you didn't get what you wanted here but it's entirely your own fault.
That's really missing the forest for the trees.
That 'other' submission may have had an unexpected problem - one, I might add, I haven't been able to reproduce myself, and apparently Graf hasn't either. And whatever the cause is, rest assured it will be fixed, but if you look at the submission - or rather what's left of it - is some superficial changes to a handful of functions, while yours completely messes up two large source files.
But one word about QZDoom:
I was a bit surprised by Graf's post that prompted this, but looking at it he seems to be right: If you want to experiment, feel free to experiment, but just looking at your repo, you are making it very hard to get stuff out of there, it's all an incoherent mix of experimentation, which should be local to your port, cleanup, which should be backported ASAP and a coding style that's not conductive to collaboration.
Just taking the RGB666 stuff is a textbook example of doing it wrong: Instead of keeping the code separate from the RGB555 code and making the selection somewhere where code consistency is less important, like the place where the drawers are being called and splitting off into two separate drawers there, you changed every single one of them separately. Your entire work basically forced Graf's hand, he'd either go along or leave you in the dust. Seems that you didn't get what you wanted here but it's entirely your own fault.