Cxbx-Reloaded / XbSymbolDatabase

Xbox Symbol Database library
MIT License
25 stars 10 forks source link

Fix LTCG Versioning Method #79

Open RadWolfie opened 5 years ago

RadWolfie commented 5 years ago

Due to D3D8LTCG's OOVPA signatures using two separate versioning which is understandable. However, the signatures with version "1024" appear are not always in 3911's database. Which I had noticed and so are the other versions too.

I suggest a better versioning method to keep the LTCG's database sane and easier to find.

https://github.com/Cxbx-Reloaded/XbSymbolDatabase/blob/76146fa05dca5ce0b472e586c6eb1e518fc32743/OOVPADatabase/D3D8LTCG_OOVPA.inl#L39-L41

My 2 proposals are:

OR


TASKS:

JayFoxRox commented 5 years ago

I've left some comments via Discord. But they didn't find their way into this issue:

Instead of hacks like this 1xxx or 2xxx, just fix it properly, please. Even kernel version 6000 would only be ~0x1770; so bits 0x2000, 0x4000, 0x8000 remain untouched.

It could become OPTIMIZED | 3911 (instead of 2xxx ~ 2911) or RECOGNIZED | 3911 (instead of 1xxx ~ 1911). So it would add flags / state to the unused bits:

You could also always add 0x2000 as another flag (or fall-back to it, if 0x8000 is already used - I didn't check); 0x2000 either as boolean, or as extension for state number, giving you 8 options when combined with 0x4000 and 0x8000.

It's basically the same as 1xxx / 2xxx (encoding multiple things in one field), except my suggestion is more readable and less error prone (avoids accidental match of last 3 digits).

Obviously, you could also turn these flags into macros: OPTIMIZED(3911) if that's considered more readable.

If the codebase already uses bitmasks in the upper bits and has no room for more flags, the entire encoding could be changed to something like VERSION_3911 (which would refer to an index, instead of a 4 digit number for the first version).

RadWolfie commented 5 years ago

Adding the flags will not help. The symbol cache doesn't have any "flag" support and shouldn't provide support for it anyway. Also, generated symbol strings are output differently for LTCG.

My second proposal is closely identical to your suggestion, @JayFoxRox. Except, it will break Cxbx-Reloaded. Meaning, Cxbx-Reloaded will need an update to match the symbol renames.

JayFoxRox commented 5 years ago

We've discussed this on Discord again.

My proposal wouldn't work, because the Version number is used as a string as part of the macro. So math can't be done:

https://github.com/Cxbx-Reloaded/XbSymbolDatabase/blob/c89ee864269711491e710ada252b0f301258443d/OOVPA.h#L160-L161

Additionally, the version field might only be 13 bit (although the comment is apparently outdated):

https://github.com/Cxbx-Reloaded/XbSymbolDatabase/blob/c89ee864269711491e710ada252b0f301258443d/OOVPA.h#L152


Since then, @RadWolfie has also clarified that the original post contains 2 different proposals. The second proposal doesn't cause these issues. My understanding is that it moves these markers into the symbol name (which is also a bit dirty, and might have some drawbacks / but also some benefits). It would essentially add the functions as a separate function handler.

RadWolfie commented 5 years ago
REGISTER_OOVPAS(D3DDevice_DrawVerticesUP, 1024, 1036, 1048, 1060),
REGISTER_OOVPAS(D3DDevice_DrawVerticesUP_12, 2024),

Convert above to below as like proposal 2

REGISTER_OOVPAS(D3DDevice_DrawVerticesUP, 3911, 4034, 4627, 5849),
REGISTER_OOVPAS(D3DDevice_DrawVerticesUP_LTCG_12, 3911),

or something @JayFoxRox suggest to use the LTCG at the end of the symbol.

REGISTER_OOVPAS(D3DDevice_DrawVerticesUP, 3911, 4034, 4627, 5849),
REGISTER_OOVPAS(D3DDevice_DrawVerticesUP_12_LTCG, 3911),

Plus he also advise to use double underlines, __ for any numbers/"LTCG" after the mangle symbol name.

RadWolfie commented 1 year ago

Just want to share my discovery when worked on D3D's render state variables pull request #177. I notice there are functions that are exactly the same except shifted by one to five offsets, sometimes more than five. Which is majority affecting LTCG titles. Due to current and future versioning' limitation, I think we should re-haul OOVPA design to something else*. Doing so will cut down redundant signatures that are only shifted one to five offsets into one simple signature. Then future versioning design could work better.

* Something else could be like tuple design which could transform into custom instruction internally. How would it work is a mystery for me. However, this type of work will require backward compatibility removal, aka OOVPA(_NO)_XREF, first.

jarupxx commented 1 year ago

Similar to RadWolfie's first proposals, but adding a minor number. It also avoids conflicts between D3D and D3D8LTCG 1xxx. Such as

D3DDevice_DrawIndexedVerticesUP, 3911
D3DDevice_DrawIndexedVerticesUP, 1024
D3DDevice_SelectVertexShader_0, 2072
D3DDevice_SelectVertexShader_0, 2084
D3DDevice_DrawIndexedVerticesUP, 3911
D3DDevice_DrawIndexedVerticesUP, 3911.1
D3DDevice_SelectVertexShader_0, 5849.1
D3DDevice_SelectVertexShader_0, 5849.2
RadWolfie commented 1 year ago

@jarupxx, I am not seeing how minor number will work with current implementation nor with group of symbols in the database list.