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

Sw blending postprocessing #672

Closed
wants to merge 23 commits into from
Closed

Conversation

gregory38
Copy link
Contributor

This PR adds a new combo box to replace accurate blending option. It also round differently the value in the shader to be more SW alike.

I think it is possible to accelerate colclip operation in the future with float texture or atomic. Unfortunately I didn't have any success currently. So I will postpone it to a later PR.

@ssakash do you have the code to
1/ add the new combo box
2/ remove the old aggressive check box

@ssakash
Copy link
Member

ssakash commented Jul 24, 2015

I have the patch for the new combo box and removal of the other accurate options. though, should I also PR a new commit for the removal of aggressive checkbox in the following branch ? I haven't done it yet. (though should be easy, just the removal of the CONTROL in Resource file)

also, I sent the patch to @refractionpcsx2 for cross checking it. I'd push it to the branch after ref takes a look at it.

@gregory38
Copy link
Contributor Author

Could you post the link to your branch? I will cherry-pick the commit. Yes add a commit that remove the box.

@refractionpcsx2
Copy link
Member

Sorry I didn't look at the patch last night, can't check tonight as I'm out, but saturday looks like the day!

@ssakash
Copy link
Member

ssakash commented Jul 24, 2015

@gregory38
Copy link
Contributor Author

You need to keep accurate date. It is a completely separated option

Edit: And there is a dedicated API for combox. Please reuse it. No need to duplicate string in acc_blend_unit

@gregory38
Copy link
Contributor Author

@ssakash sorry to urge you.

I would like to merge the branch as soon as possible so the new opion land in time for next Ubuntu release. They will freeze it soon. Besides, I have another round of updates (to accelerate the high level). So could you update your patch based on the above feedback. Thanks you.

@ssakash
Copy link
Member

ssakash commented Jul 26, 2015

Yeah, I will get to it. sorry for the delay, I wasn't on git yesterday. just looked at the patches and suggestions, will update it soon. :)

@ssakash
Copy link
Member

ssakash commented Jul 26, 2015

@gregory38
here's the new branch with the updates: https://github.com/ssakash/pcsx2/tree/patch-41

also about the duplicate string, It's recommended to use 'CB_ADDSTRING' when working with combo box on resource files for better sorting. also it would be conflict for future CBS_SORT implementation. I think the current implementation is fine :)

@gregory38
Copy link
Contributor Author

I mean the GSdx API.

ComboBoxInit(IDC_..., m_gs_acc_blend_level, theApp.GetConfig("....", 1));
            if(ComboBoxGetSelData(IDC_...., data))
            {
                theApp.SetConfig("....", (int)data);
            }

The idea is that sprites are often use for post-processing effect (ofc except 2D games)

Most of the time post-processing supports SW blending with a small speed penality. SW
blending is more accurate so it is better to use it.
It is much easier to configure this way
Speed penality is small (only GPU) but it is more accurate
It improves SotC blooming. Suikoden seems to be fine too.
The updated medium level will run for all sprites. It helps sotc blooming effect and it remains
fast enough to be enabled by default (at least on 3D games)
The new high level will run for all sprites + color clipping
Do DATE algo selection before blending. This way we can detect bad
interaction.

Regroup all blending/colclip in a single block.  Avoid to check abe &&
rt multiple times.
Basically the code does the alpha multiplication in the shader therefore
the blend unit only does a pure addition. This way the multiplication is
accurate and accurate_blending doesn't requires a costly barrier.

This code also avoid variable duplication to make the code more separated.
Hopefully blending can be done in a separated function

It is preliminary work to support fast color clipping with HDR

v2: fix assertion compilation failure
Previous CopyRect function does a memcopy without conversion.

This function will allow to use different format for input/output. Just a
possibility for the future
Such as GL_RGBA32F/GL_RGBA16F/GL_RGBA16UI/GL_RGBA16I/GL_RGBA16

Not yet used
It will avoid texture rouding error with negative number
@ssakash
Copy link
Member

ssakash commented Jul 26, 2015

I see, didn't know about these functions. by the way, updated the branch. please check it out : https://github.com/ssakash/pcsx2/tree/patch-41

gregory38 and others added 3 commits July 26, 2015 18:02
The following patch merges all the Accurate options related to the blending unit into a single one.
The option is pretty much useless, CRC hack level controls the usage of hacks already.
@gregory38
Copy link
Contributor Author

Thanks I merged your commit in a branch and fix a potential regression. Time to test it :)

@gregory38
Copy link
Contributor Author

@prafullpcsx2 I found a regression that I will fix tonight.

Those cases don't need the Cd addition and were already optimized anyway

Fix a regression on GoW2
@gregory38
Copy link
Contributor Author

@refractionpcsx2 could you prove a build of this branch for prafull and others?

@refractionpcsx2
Copy link
Member

Sure here you go http://www.filedropper.com/gsdx32-sse2-postproc2707_1

Edit: link updated with tidy up stuff

@refractionpcsx2
Copy link
Member

Looks to work okay (the gui stuff) Tidy'd the options up a bit tho, it was all over the place :P

@refractionpcsx2
Copy link
Member

Games seem to be fine, GoW2 looks nice apart from the bar across the middle of the screen :)
And of course you know about the SotC glitchyness

@ssakash
Copy link
Member

ssakash commented Jul 28, 2015

Looks to work okay (the gui stuff) Tidy'd the options up a bit tho, it was all over the place :P

Yep, it was quite crowded. :p that's why I suggest a landscape dialog box.

@gregory38
Copy link
Contributor Author

Games seem to be fine, GoW2 looks nice apart from the bar across the middle of the screen :)

What do you mean? Is it a regression, did you enable CRC to minimum, the game needs it.

 And of course you know about the SotC glitchyness

Could you try to add a

glFinish();

in the end of the GSRendererOGL::SendDraw function?

@refractionpcsx2
Copy link
Member

Ill try later, I'll probably be in about 11pm tonight, so ill give it a go.

As for GoW2 I didn't try that no, ill give it a shot.

@gregory38
Copy link
Contributor Author

Ok. If you can also test the hdr-colclip branch. I change a barrier, maybe it is the reason. Still on this branch, I need another test ;)

There is a glFinish that kill my perf in the code (bottom of GSRendererOGL). It feels like a driver bug could you test blending unit high with/without the glfinish. Check SotC shadows, you will quickly see the issue.

@refractionpcsx2
Copy link
Member

I don't think I'll do all that tonight :P But I'll certainly look at it.

@gregory38
Copy link
Contributor Author

sure :)

@prafullpcsx2
Copy link
Contributor

@gregory38 I am downloading VS2013 iso today and also making room on my HDD so most probably by tomorrow I will have things setup to compile on my own. Then I can quickly test out your new changes.

@gregory38
Copy link
Contributor Author

That a great news for Ref ;)

@refractionpcsx2
Copy link
Member

wahoo! \o/ :P

@refractionpcsx2
Copy link
Member

OK the screen mess on SotC is gone with the hd-colclip branch and i see what you mean about it being corrupted without the glFinish in there. Is the behaviour different on AMD cards then?

I also found the version without the finish to be slower (possibly due to the corruption though)

Edit: It's also slightly quicker if you put it in the end of the DoFirstPass with this code

if (require_barrier) glFinish();

Seems to still work on the shadows but slightly faster, also the colclip = 1 doesn't get called in this instance (putting it in there doesn't help :P )

Purpose is to allow the separation of blending code into a dedicated function
@gregory38
Copy link
Contributor Author

Cool for the corruption. I think the improvement come from the barrier commit so I integrate it to this branch.

Yes colclip == 1is the old broken method. The HDR (as high dynamic range) version is much better but slower (mostly due to the glfinish).

I asked some help on the openGL forum in case I did something wrong. In the meantime, we need to find someone with an AMD GPU to test the behavior. On my side, I will try to replace the glFinish primitive by https://www.opengl.org/wiki/GLAPI/glWaitSync . I hope it will be lighter.

@gregory38
Copy link
Contributor Author

@refractionpcsx2 could you give a quick test to ensure corruption is gone with this branch (sw-blending-postprocessing). If it is good, I will merge it :)

@gregory38
Copy link
Contributor Author

Ok. A good guy confirms me that corruption is gone on suikoden. I will rebase the branch to squash a couple of commit and then I will merge it.

@gregory38 gregory38 closed this Jul 30, 2015
@gregory38 gregory38 deleted the sw-blending-postprocessing branch July 30, 2015 16:26
@refractionpcsx2
Copy link
Member

So shall i check the SotC corruption on master or just be happy that it'll be sorted with the hdr-colclip branch? :p

@ssakash
Copy link
Member

ssakash commented Jul 30, 2015

A good guy already tested it. :)

@gregory38
Copy link
Contributor Author

Master must be fine now. I'm currently cleaning the hdr branch. I will drop the old colclip algo, code is really a complete mess.

@karasuhebi
Copy link
Contributor

In the meantime, we need to find someone with an AMD GPU to test the behavior.

What do you need me to test? Is it merged to master by now?

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.

None yet

5 participants