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

GSdx: Palette management refactor and fix (v2) #2731

Merged
merged 9 commits into from Dec 11, 2018

Conversation

Projects
None yet
4 participants
@AlessandroVetere
Copy link
Contributor

commented Dec 2, 2018

Refactor and fix palette management.

  1. Improve ICO hack by using palette cache for depth sampling, removing only point of creation of palette textures outside TC (removes conditional deletion of Source::m_palette in ~Source()),
  2. Purge comments,
  3. Reformat parts of the code,
  4. Centralize CLUT comparison logic in PaletteKey equality operator,
  5. Fix and rationalize palette texture or clut copy attachment and update with respect to pre-PaletteMap behavior,
  6. Make PaletteMap capable of managing at the same time Palette with and without GSTexture,
  7. Handle possible case of Source lookup hit with psm.pal value changed.

Closes #2702 , Closes #2736 and prevents possible similar regressions.

@lightningterror lightningterror added this to the Release 1.6 milestone Dec 2, 2018

@AlessandroVetere AlessandroVetere force-pushed the AlessandroVetere:refactor-palette branch 2 times, most recently from 41818fd to fdbc37d Dec 2, 2018

}
else {
src->m_texture = m_renderer->m_dev->CreateTexture(tw, th);
// by default: src->m_should_have_tex_palette = false;
AttachPaletteToSource(src, psm.pal, false); // Attach only Palette object and clut copy
if (psm.pal > 0) {

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 4, 2018

Author Contributor

This change is most likely what fixed #2702, as we were malloc'in 0 bytes clut arrays without the psm.pal > 0 check.

This comment has been minimized.

Copy link
@orbea

orbea Dec 4, 2018

Contributor

No, that change doesn't seem to affect Tales of Legendia.

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 4, 2018

Author Contributor

Oh strange. Could you please try again the last version of PR plugin to see if the regression is still fixed? I applied some modifications to make things easier to manage.

This comment has been minimized.

Copy link
@orbea

orbea Dec 4, 2018

Contributor

Yes, I applied the whole PR as a patch to master and both Tales of Legendia and Tales of the Abyss still seem fixed with and without 8 bits textures.

return false;
}

ASSERT((lhs.pal & 15) == 0);

This comment has been minimized.

Copy link
@gregory38

gregory38 Dec 9, 2018

Contributor

Why the assert ? It doesn't make any sense here

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 9, 2018

Author Contributor

Yeah maybe you are right there is already the ASSERT in the GSVector4i::compare64, I can remove it if needed

This comment has been minimized.

Copy link
@gregory38

gregory38 Dec 9, 2018

Contributor

yes because this assert belong to compare64 not this function.

@@ -1451,6 +1452,7 @@ GSTextureCache::Source* GSTextureCache::CreateSource(const GIFRegTEX0& TEX0, con
{
src->m_texture = m_renderer->m_dev->CreateTexture(tw, th, Get8bitFormat());
src->m_should_have_tex_palette = true;
AttachPaletteToSource(src, psm.pal, true);

This comment has been minimized.

Copy link
@gregory38

gregory38 Dec 9, 2018

Contributor

Not related to this line of code in particular.
Is there a possibilities that we attach the palette twice ?
We attach in Create Src and in LookUp Src.

Maybe it could be possible to avoid the compare64 check in lookup ? It isn't critical if current code is safe but potentially a bit "slower".

This comment has been minimized.

Copy link
@gregory38

gregory38 Dec 9, 2018

Contributor

it feels like the last commit, isn't it ?

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 9, 2018

Author Contributor

You are right, this commit alone introduces double palette attachment (as it invalidates the current fast path), but with the last commit the fast path is managed in an easier way and it always avoid double attachment in Create Src and LookUp Src.

This comment has been minimized.

Copy link
@gregory38

gregory38 Dec 9, 2018

Contributor

I think the best will be to do a single commit

  • that attach palette in all cases / as soon as possible
  • the bool new_source of latest commit (which I think is the fix to double attachment)

This way we have an atomic (and logical) commit.

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 9, 2018

Author Contributor

I agree with that, the intermediate commit breaks fast path so it's better to squash them in one.

void GSTextureCache::Palette::UploadClut() {
if (!m_tex_palette) {
m_tex_palette = m_renderer->m_dev->CreateTexture(256, 1);
m_tex_palette->Update(GSVector4i(0, 0, m_pal, 1), m_clut, m_pal * sizeof(m_clut[0]));

This comment has been minimized.

Copy link
@gregory38

gregory38 Dec 9, 2018

Contributor

Is it expected that UploadClut will upload only once ?

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 9, 2018

Author Contributor

Yes because the clut content for a given Palette is copied at object creation and never changed, so also the palette need only to be uploaded once (possibly lazy upload)

This comment has been minimized.

Copy link
@gregory38

gregory38 Dec 9, 2018

Contributor

In this case, UploadClut could have a better name. Create/Initialize (once) vs Upload (can be repeated).

You can also reduce the size of the texture. 256 is too big for small m_pal.

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 9, 2018

Author Contributor

Ok for name. As for Tex size, IIRC I tried and creating smaller textures lead to various errors. Will test again.

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Dec 9, 2018

Author Contributor

Yeah creating texture with 16 as width breaks the plugin (both dx and ogl), while for big pal (256) it is good (as hardcoded in there). However it would be an interesting enhancement to create smaller textures for this use case.

@gregory38

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

It is much better with small commits. Small comments but it is fine for me. It requires a couple of final tests. And it should be fine.

@AlessandroVetere AlessandroVetere force-pushed the AlessandroVetere:refactor-palette branch from 9514a6d to b18702d Dec 10, 2018

AlessandroVetere added some commits Dec 10, 2018

GSTextureCache: Refactor and fix palette management (v2).
1) Refactor palette comparison fast path mechanism to avoid using mutable member of Source,
2) Attach palette to texture as soon as needed,
3) Bugfix attach CLUT copy only when pal > 0.
GSTextureCache: Initialize palette texture if needed and not done yet.
Also comment on the reason behind palette texture is always created with size 256x1.

@AlessandroVetere AlessandroVetere force-pushed the AlessandroVetere:refactor-palette branch from b18702d to db75b8f Dec 10, 2018

@lightningterror

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

The crash mentioned in #2702 is reproducible in variety of dumps/games tested, oddly enough the crash is reproducible if a game utilizes depth, now since dx11 got depth convert as well it's also reproducible there.

@gregory38 if the PR has small nitpicks to be taken care of I suggest we postpone them for another PR, this one is rather really important and I'd like to see it merged as fast as possible.

@gregory38

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

No need to rush man.

Code is fine for me. It can be merged.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

No need to rush man.

I don't like crash regressions 😄

@gregory38

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

Then don't merge broken code in the first place ;)

@lightningterror lightningterror merged commit fe845ae into PCSX2:master Dec 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@AlessandroVetere AlessandroVetere deleted the AlessandroVetere:refactor-palette branch Dec 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.