Less graphical glitches in SRWMXP #2297
Conversation
… so) CLUTs. Still some notable blending errors.
| ConvertColors(clutBufConverted_, clutBufRaw_, getClutDestFormat(clutFormat), clutTotalBytes_ / sizeof(u16)); | ||
| void *dstPtr = (u16 *)clutBufConverted_ + clutBase; | ||
| // This magically works! | ||
| int numPixels = clutTotalBytes_ / 2; |
There was a problem hiding this comment.
Hmm, I'm not sure that "magically works" is a good thing. I think the code is much clearer without hardcoding a 2 here. Why a 2? 2 colors? Magic numbers suck. sizeof(u16) has meaning.
So it seems like what you're doing here is running ConvertColors() offset by clutBase (plus a bunch of probably unnecessary changes), right?
If that's correct, it makes me think that LoadClut() should work differently too. If clutTotalBytes_ does not include clutBase, doesn't that mean it's not reading enough memory in? Otherwise, you're using old palette values or something?
-[Unknown]
There was a problem hiding this comment.
Yes, this game reads from there.
It appears to me that the games update the register with how many bytes they will be using. My alternative fix map the read/write locations but that will break some text ("LEVEL-UP!") in SRWZ2.
I also changed what content to hash.
|
Fair warning: if you break things again and say "Just go ahead and fix it." about your own work, I'm gonna be pissed. -[Unknown] |
|
Sorry for last time's hassle. |
|
This fixes the texures in SRWMXP: |
| void *dstPtr = (u16 *)clutBufConverted_ + clutBase; | ||
| // This magically works! | ||
| int numPixels = clutTotalBytes_ / sizeof(u16); | ||
| ConvertColors(dstPtr, srcPtr, getClutDestFormat(clutFormat), numPixels); |
There was a problem hiding this comment.
Does it work instead to do:
const u32 clutBase = (gstate.clutformat & 0x1f0000) >> 12;
const u32 totalBytes = clutTotalBytes_ + clutBase;
And then just replace all the clutTotalBytes usage inside this function with totalBytes? I'm not really sure how that could work, but that sounds like what you're doing.
But, it would seem like LoadClut() doesn't load enough bytes in that case. I mean, think of it this way:
LoadClut has a contract that it populates clutBufRaw_[0 .. clutTotalBytes_] (looking at it in bytes.) That means anything > clutTotalBytes_ is out of bounds.
With your changes:
void *srcPtr = (u16 *)clutBufRaw_ + clutBase;
int numPixels = clutTotalBytes_ / sizeof(u16);
ConvertColors(dstPtr, srcPtr, getClutDestFormat(clutFormat), numPixels);
You are reading from (again looking at it as bytes) clutBufRaw_[N .. N + clutTotalBytes_]. If N is greater than 0, you are reading uninitialized RAM, am I wrong? It's outside the bounds of what was read.
I think this means one of:
- The solution here is that you're no longer running
ConvertColors()on a portion of the palette that is being used. This means either it's being double-applied or applied incorrectly. - The uninitialized bytes happen to have useful data in them from a previous clut load, but we shouldn't be reading them.
- ClutLoad() should be reading more bytes, or even a constant number of bytes to ensure these bytes are all initialized.
I don't think the right solution is to read uninitialized memory.
-[Unknown]
There was a problem hiding this comment.
In the scenes where there were the colour issues, this game first loads whole 0x400 bytes, then loads 0x20 (which is exactly how many it will use from the 0x400 bytes) "unused" bytes before setting the texture. The similar thing happens in SRWZ2 too.
EDIT: I'll try your suggested change.
There was a problem hiding this comment.
It actually calls GE_CMD_LOADCLUT after setting the base with a value of 1?
That implies that GE_CMD_LOADCLUT respects the clutformat parameters after all (at least the base.) If yes, then we have to load into and read from the offset in LoadClut(), and we'll need a gstate_c.clutFormatChanged or something rather than just diff. Might even affect flushing...
But it's hard to tell without having the game to debug. I don't think I have any games that actually use that clutBase.
-[Unknown]
There was a problem hiding this comment.
With your change it works, too.
There was a problem hiding this comment.
Darn, that probably means loading the clut needs to respect the offset. If true, this means LoadClut() will have to care about clutBase, but hopefully flushing won't be an issue... I'm not sure. Changes to clut format should already flush....
-[Unknown]
|
In that case, does something like this work? https://github.com/unknownbrackets/ppsspp/compare/clut-base Had to kill the optimization of not rehashing except after initial clut load when format changed, but I think that was not common enough to optimize for anyway. Final Fantasy 2 and Final Fantasy Tactics (which care about things like that) still work. Changed clutBufRaw_ to u8 to make offsetting clear and simple, probably follows clutTotalBytes_ better. The above assumes that load and use are all offset by base. -[Unknown] |
|
@unknownbrackets: That also fixes MXP but breaks the title screen of Z2. I had tried a similar (not exactly the same) attempt which resulted in the same problem. |
|
What does the frame dump look like for Z2 and for MXP? Does it help to load and update the clutBase bytes too (that would be really strange, but I guess it could make sense)? -[Unknown] |
|
Frame dumps with your code: There was also an old frame dump of MXP in #1060#issuecomment-18894167. And here's the "alternative fix" I mentioned earlier. It was an experimental hack: It roughly did the same thing (only concerning what to load) as yours, except delayed until UpdateCurrentClut(). EDIT: I don't get what you mean "to load and update the clutBase bytes" (or "clutBaseBytes"?). Do you mean to update the corresponding bits in gstate.clutformat? |
|
Well, my change above makes it load the slice of the clut from base -> base + size. Another option is loading 0 -> base + size. That's what I'd meant. MXP seems to:
So based on the byte ranges, it doesn't sound like it should account for the offset after all, but who knows. It seems strange that the game would do this anyway... So then, what about this? https://github.com/unknownbrackets/ppsspp/compare/clut-base That should be safe, although it may be slow if a game uses base in a way that doesn't just go outside the previously loaded bytes (if those bytes change, we'll cache a new texture each time...) Or alternatively, could load the base + size.... May need to do tests on a PSP trying different cluts to be sure how this works... -[Unknown] |
|
Unfornaturely i don't have MXP to test it but tested the above code towards SRWZ2 and seems to be fine and no broken .(BTW , compile succeed need clutBufRaw_ = new u32[4096 * 4]; // 16KB) Looking forward the testing result from @aquanull on MXP |
Did you change the .h file too? I changed it to a -[Unknown] |
|
Sorry .Just changed ,h as well and it is fine now , |
|
@unknownbrackets @raven02: Yes it works and looks safer. It is still weird that the actually used bytes are not what were loaded just now. |
|
Yeah, but it's unfortunately still hashing unintialized bytes. It seems like the game is reading that part of the clut. I guess I just have to trust that not many games use this... -[Unknown] |
|
This still only try to do the minimal jobs, assuming that the games always send GE_CMD_LOADCLUT whenever they specify the actual palette entries to be used. If that's not true, then neither this nor #2303 is safe because both rely on TextureCache::clutTotalBytes_ that is only updated when executing GE_CMD_LOADCLUT. EDIT: I wonder if we could remove TextureCache::clutTotalBytes_. |
|
Sorry I forgot about this one. Is it good to go? |
|
I don't think this hashes properly. Wasn't the game fixed with the other change? -[Unknown] |
|
Can this be fixed or should I close this pullrq? |
|
Ping? @aquanull |
...by updating the right (or seemingly so) CLUT palette entries. Partially solves #1060.
This may seem weird as it reads bytes from a location different from where they were written to, but this works without breaking other games like SRWZ2 (which my alternative fix breaks some text in it).