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

force FixFpuLocations in Indiana Jones gfx issues #251

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@ghost
Copy link

commented Mar 9, 2015

No description provided.

@AmbientMalice

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2015

This seems to break Shadow Man. So as much as I'd like it to be added, I'm a bit... uneasy with its current state.

@cxd4

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2015

That shouldn't be all that surprising. He knows that this fix is in the form of taking out the broken implementation, but removed/no implementation at all is not necessarily better than an existing but broken one. So this commit is more a bypass rather than a genuine fix; formal investigation there on what really should happen is still pending.

@Frank-74

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

When the FR bit in the Status register equals zero, the Floating-Point general purpose registers (FGRs) are 32-bits wide.
To retain single-precision floating-point format data, sixteen even number registers out of thirty-two FGRs can be accessed.
To retain double-precision floating-point format data, even number registers are used for low-order bits of data, and odd number registers for high-order bits.
The registers are used as even-odd pairs, and can retain sixteen double-precision format data.

When the FR bit in the Status register equals one, the Floating-Point general purpose registers (FGRs) are 64-bits wide.
To retain single-precision floating-point format data, low-order bits of thirty-two FGRs are used.
To retain double-precision floating-point format data, thirty-two FGRs are used.

@cxd4

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

It's been years since I've read that manual.
I remember pretty much skimming through the CP1 stuff since I mostly cared just about the R4000 basics.

NEC also has some diagrams of the FR Status bit in their manual on their modifications to the R4300i:
http://datasheets.chipdb.org/NEC/Vr-Series/Vr43xx/U10504EJ7V0UMJ1.pdf

Searched over the whole thing and dug back up a few interesting pages.

Page 166 is a diagram of the CP0 register Status, showing the bit encodings describing the FR bit.

Page 208 tells you the 3 formats that a floating-point register can be accessed during an operation, with a VERY helpful diagram on the next page 209 showing the visual layout when FR=0/1.

FPR is physically configured with General Purpose registers (FGRs). When the
FR bit in the Status register equals 0, the FPR is configured with two 32-bit FGRs.
When the FR bit in the Status register equals 1, the FPR is configured with a single
64-bit FGR.

If the FR bit of the Status register is set to 1, all the thirty-two 64-bit floating-point
registers defined by the MIPSIII architecture can be accessed. If this bit is cleared,
the processor accesses the sixteen 64-bit floating-point registers defined by the
MIPSII architecture.

And everything else I think is repeated and re-stated junk throughout each of the COP1 op-codes.

@Frank-74

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

From the same manual, chapter 17, pages 557/558, shows 2 routines ( 32bit and 64bit ) to retrieve the value of an FPR or to change the value of an FGR.

32 Bit Mode
value <-- ValueFPR(fpr, fmt)
    /* undefined for odd fpr */
    case fmt of
        S, W:
            value <-- FGR[fpr+0]
        D:
            value <-- FGR[fpr+1] || FGR[fpr+0]
    end

StoreFPR(fpr, fmt, value):
    /* undefined for odd fpr */
    case fmt of
        S, W:
            FGR[fpr+1] <-- undefined
            FGR[fpr+0] <-- value
        D:
            FGR[fpr+1] <-- value63...32
            FGR[fpr+0] <-- value31...0
    end

64 Bit Mode
value <-- ValueFPR(fpr, fmt)
    case fmt of
        S, W:
            value <-- FGR[fpr]31...0
        D, L:
            value <-- FGR[fpr]
    end

StoreFPR(fpr, fmt, value):
    case fmt of
        S, W:
            FGR[fpr] <-- undefined32 || value
        D, L:
            FGR[fpr] <-- value
    end
@cxd4

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

@AmbientMalice you mentioned at the bottom of this post that the issue is improved on 1.4:
http://forum.pj64-emu.com/showpost.php?p=59962

I mostly just tested right after the menu when you press start, since what you were taking a screenshot of in that post wasn't something I knew how to get to as quickly, but, I had a look at the way this function was being done in Project64 1.4 source:

void SetFpuLocations (void) {
    int count;

    if ((STATUS_REGISTER & STATUS_FR) == 0) {
        for (count = 0; count < 32; count ++) {
            FPRFloatLocation[count] = (void *)(&FPR[count >> 1].W[count & 1]);
            //FPRDoubleLocation[count] = FPRFloatLocation[count];
            FPRDoubleLocation[count] = (void *)(&FPR[count >> 1].DW);
        }
    } else {
        for (count = 0; count < 32; count ++) {
            FPRFloatLocation[count] = (void *)(&FPR[count].W[0]);
            //FPRDoubleLocation[count] = FPRFloatLocation[count];
            FPRDoubleLocation[count] = (void *)(&FPR[count].DW);
        }
    }
}

(Wow that looks sexy it's even written in C!)

The key difference between this and how FixFpuLocations is done after pj64 1.4, up till current 2.x, is that under the else it reads (void *)(&FPR[count].W[0]); instead of &m_FPR[count].F[1]; (address of first 32-bit half instead of the address of second 32-bit half) (the W and F mean the same data size so doesn't matter). However if I change from 1 to 0 that doesn't fix anything at all.

Since the function is otherwise identical in both Project64 1.4 and Project64 2.x, I fear that the success of this hack fix may be a sign of only a completely unrelated interpreter bug somewhere else, which might be causing STATUS_FR to not get set when it should have been set.

@AmbientMalice

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

@cxd4 I'm glad my testing was helpful, even if the results aren't exactly desirable - I'm aware of how troublesome fixing interpreter bugs can be.

@Frank-74

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2015

Maybe this could be made an rdb option?

@AmbientMalice

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2015

@Frank-74 That occured to me, also. It wouldn't help PJ64's reputation as an emulator that fixes shit with hacks. There's a risk that a half-assed RDB fix would reduce the incentive to find out the real cause.

On a related note, I wish the Trading Post CPU recompiler freeze could be fixed. Either fix the recompiler or use a cheat to bypass the Trading Post. (The latter is terribly hacky, but it's plausible since the game bypasses the TP itself if the level has already been completed.)

Naboo and Infernal Machine are very close to working satisfactorily on PJ64. GLideN64 will emulate their graphics correct, even if the LLE mode isn't as fast as it could be. We're on the verge of PJ64 emulating games "out of the box" many folks gave up on years ago.

Oh, yea, and the RSP recompiler needs fixing, too because it freezes with Naboo\Indy.

@theboy181

This comment has been minimized.

Copy link
Contributor

commented May 2, 2015

I was looking at this game today, and its like the camera is in the wrong location? Is there a plugin that allows you control the position of the camera. I would like to see how this game looks with a proper camera position.

@AmbientMalice

This comment has been minimized.

Copy link
Contributor

commented May 2, 2015

@theboy181 The camera position is just a side effect. You need to compile PJ64 with the FixFPULocations hack.

@ghost

This comment has been minimized.

Copy link
Author

commented Aug 22, 2015

I can't solve this issue without Dirty hacks.
Must I close this pull request?

@AmbientMalice

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2015

@exhalatio I think @LegendOfDragoon was taking a look at the issue, but I dunno whether he was making any progress. I don't think there's any need to close the issue.

@cxd4

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2015

We want to get to reverse-engineering it first and use this as an opportunity to compare emulator to hardware to game results; that's probably why this hasn't already been closed--a reminder.

@ghost

This comment has been minimized.

Copy link
Author

commented Aug 23, 2015

It makes sense.

@LegendOfDragoon

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2015

@exhalatio I think @LegendOfDragoon was taking a look at the issue, but I dunno whether he was making any progress.

Nothing conclusive yet, but I'm attempting to track down multiple issues.

@Frank-74

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2015

@AmbientMalice

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2015

@Frank-74 I'm 50/50 on using a hack. Regardless, it shouldn't be merged until the CPU interpreter/recompiler freeze bugs are fixed, IMO.

@Frank-74

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2015

I got tired of building a separate exe that is only useful for 2 games.

@cxd4

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2015

The trick is that finding the genuine fix is impossible for anyone inexperienced with the CPU ISA.

Regressions in interpreters are always far harder to fix than regressions in recompilers: You have nothing to cross-reference with that is any more accurate, save for hardware reversing which you will have to do. Basically, your cheat sheet is gone, and all one can do otherwise is guess unconfirmable workarounds.

@project64

This comment has been minimized.

Copy link
Owner

commented Apr 25, 2017

c7f8957

this should be correct fix now

@AmbientMalice

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

Seems to work now. Good work. PR creator has disappeared so you'll have to close this yourself.

Game still crashes/freezes at the end of each mission, though.

@Frank-74

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

Works perfectly now.

This and #76 can be closed now.

@project64 project64 closed this Apr 25, 2017

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.