hrydgard / ppsspp

A PSP emulator for Android, Windows, Mac and Linux, written in C++. Want to contribute? Join us on Discord at https://discord.gg/5NJB6dD or just send pull requests / issues. For discussion use the forums at forums.ppsspp.org.
https://www.ppsspp.org
Other
11.37k stars 2.18k forks source link

T&L issue : Final Fantasy Type 0 #1410

Closed dbz400 closed 11 years ago

dbz400 commented 11 years ago

This T&L issue happens recently and randomly , the four corner will overbright , it didn't happen in 0.7 version . Not too sure @cinema , @hrydgard or @unknownbrackets has any idea?

1

dbz400 commented 11 years ago

Narrow down this issue starting from Apr 8 build :)

dbz400 commented 11 years ago

Confirm broken at v0.7-543-gdea37e5 - Merge pull request #1213 from unknownbrackets/dlist-cycles (Although i don't understand why it will cause this overbright effect randomly .....but it did )

Last working one is v0.7-537-g57dabfb

unknownbrackets commented 11 years ago

Can you post a screenshot of that scene from the actual game? Maybe it's applying that effect (and was applying no effect at all before) correctly, but it's too strong?

1213 was unfortunately a sorta large change, I tried to match it as closely as possible. Can you try to find which of the commits in that pull caused it?

-[Unknown]

dbz400 commented 11 years ago

This is actually actual gameplay and this #1213 changes is pretty big , bit difficult to bisect :)

It is working fine previously until #1213

1

dbz400 commented 11 years ago

There are 5 small commits in #1213. I will try to test each one to see which broken

dbz400 commented 11 years ago

@unknownbrackets

Confirmed the offending one is 57770db - ets Delay GPU signals and waits to simulate cycles.

Revert it fixes the issue

dbz400 commented 11 years ago

@unknownbrackets , i narrowed down a bit , it is __GeTriggerInterrupt , replacing it with previous revision and fix the issue

unknownbrackets commented 11 years ago

It's possible this is like the Wipeout bloom issue and being overapplied.

Or, it's because estimation is too high.

-[Unknown]

dbz400 commented 11 years ago

If anything code change need me to test it for __GeTriggerInterrupt or vertex cost estimation , let me know .

I tried vertexcost for both small value like 1 and large value like 100 , didn't help

cyclesExecuted += vertexcost * count;

unknownbrackets commented 11 years ago

Well, first off, there are two places - GPUCommon and GPU_GLES. If you comment them both out, is it the same as before?

If yes, which one triggers it? (most likely the GPU_GLES one.) Does it help to multiply count by a smaller number, like 10? (note that such a number would DEFINITELY break God of War again.)

If commenting out doesn't make it the same as before, it may be that it still isn't hitting CoreTiming soon enough. Hmm.

But this is all assuming that the game shouldn't be rendering that. I don't understand how timing could make it render something out of the blue, it must at least have code to render that effect, right? If timing is changing lighting that's definitely a bad problem.

-[Unknown]

dbz400 commented 11 years ago

Just tried 1st which is comment out both like below

-GPUCommon

// Rough estimate, 2 CPU ticks (it's double the clock rate) per GPU instruction.
cyclesExecuted += 2 * (pc - cycleLastPC) / 4;
cycleLastPC = newPC == 0 ? pc : newPC;

-DisplayListInterpreter

        {
            transformDraw_.SetupVertexDecoder(gstate.vertType);
            // Rough estimate, not sure what's correct.
            int vertexCost = transformDraw_.EstimatePerVertexCost();
            //cyclesExecuted += vertexCost * count;
            return;
        }

Both commented out and tested ,didn't help .

Will try to multiple 10 to count

dbz400 commented 11 years ago

Tried mutilple 10 to count like this , didn't help as well .

        framebufferManager_.SetRenderFrameBuffer();
        if (gstate_c.skipDrawReason & (SKIPDRAW_SKIPFRAME | SKIPDRAW_NON_DISPLAYED_FB))
        {
            transformDraw_.SetupVertexDecoder(gstate.vertType);
            // Rough estimate, not sure what's correct.
            int vertexCost = transformDraw_.EstimatePerVertexCost();
            cyclesExecuted += vertexCost * count * 10 ;
            return;
        }
dbz400 commented 11 years ago

Lastly , restore __GeTriggerInterrupt that fixes the issue

FROM

bool __GeTriggerInterrupt(int listid, u32 pc, u64 atTicks) { u64 userdata = (u64)listid << 32 | (u64) pc; CoreTiming::ScheduleEvent(atTicks - CoreTiming::GetTicks(), geInterruptEvent, userdata); return true; }

TO

bool __GeTriggerInterrupt(int listid, u32 pc) { // ClaDun X2 does not expect sceGeListEnqueue to reschedule (which it does not on the PSP.) // Once PPSSPP's GPU uses cycles, we can remove this check. DisplayList* dl = gpu->getList(listid); if (dl != NULL && dl->subIntrBase < 0) return false; GeInterruptData intrdata; intrdata.listid = listid; intrdata.pc = pc; ge_pending_cb.push_back(intrdata); __TriggerInterrupt(PSP_INTR_HLE, PSP_GE_INTR, PSP_INTR_SUB_NONE); return true; }

unknownbrackets commented 11 years ago

What if you put it back without the ClaDun return false; part? Again, reverting that will hugely impact at least a few other very popular games, so it's probably not worth doing just to fix a lighting bug. So that's not really the option we want. And I know that changing that one function will generate significantly different behavior.

If it didn't help to comment it out, it's possibly not estimation. You can try adding CoreTiming::Advance() to the.. erm... maybe end of the functions in sceGe, especially sceGeListUpdateStallAddr() and sceGeListEnqueue(). I'm not sure that's not really a solution either, especially sceGeListUpdateStallAddr(), since it may significantly impact performance too. But it'd be interesting if it changed it. Also calling CoreTiming::Advance() in those places could do funny things, I'm not sure.

-[Unknown]

dbz400 commented 11 years ago

Put it back without ClaDun return false . also works okay and fix the issue :)

bool __GeTriggerInterrupt(int listid, u32 pc) { GeInterruptData intrdata; intrdata.listid = listid; intrdata.pc = pc; ge_pending_cb.push_back(intrdata); __TriggerInterrupt(PSP_INTR_HLE, PSP_GE_INTR, PSP_INTR_SUB_NONE); return true; }

dbz400 commented 11 years ago

@unknownbrackets , so if it's okay to use back the old __GeTriggerInterrupt without ClaDun ..... return false ?

unknownbrackets commented 11 years ago

Not really, the PSP definitely does not trigger interrupts the moment the list is enqueued before the pc even gets there. This can easily be verified on the PSP. We can have no doubt that changing it to the old code would be completely incorrect, plus it would break ClaDun X2 and possibly other games again.

If the Advance thing didn't help either, then it's most likely that a GE command is being applied incorrectly imho.

-[Unknown]

dbz400 commented 11 years ago

Sure.then would be better leave it to you to get proper fix from GE side

dbz400 commented 11 years ago

I think this issues have been all fixed .Closing it .