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

Dump types per module #58

Merged
merged 10 commits into from
Aug 30, 2024
Merged

Conversation

Cre3per
Copy link

@Cre3per Cre3per commented Jul 26, 2024

#57

depends on #47

❌ Tested

I don't have a cheat to test these changes with, could someone else try porting their cheat to the new format and report back?
Particular focus should be paid to static member access functions, because they now access some library rather than every single one, and I'm not sure if static members are shared between libraries.

Preview of the new sdk structure

...
├── tier2
│   ├── CRangeFloat.hpp
│   └── CRangeInt.hpp
├── vphysics2
│   ├── constraint_axislimit_t.hpp
│   ├── constraint_breakableparams_t.hpp
│   ├── constraint_hingeparams_t.hpp
│   ├── IPhysicsPlayerController.hpp
│   └── vphysics_save_cphysicsbody_t.hpp
└── worldrenderer
    ├── AggregateLODSetup_t.hpp
    ├── AggregateMeshInfo_t.hpp
    ├── AggregateSceneObject_t.hpp
...

@Cre3per Cre3per marked this pull request as draft July 26, 2024 14:41
@Cre3per
Copy link
Author

Cre3per commented Jul 26, 2024

marked as draft until #47 is merged

@Cre3per
Copy link
Author

Cre3per commented Jul 27, 2024

There's a semi-intentional bug in this code where find_required_modules(type) does not return modules required by template arguments, and template arguments are underqualified.

For example, suppose Vector was declared in the module "container" and "Int" was declared in the module "number".

"Vector" turns to "container::Vector" or "Vector" (I'm not sure how well source2 handles template lookup), but not into "container::Vectornumber::Int", which would be correct.
The include emitted for this type is "container.hpp" or none (Again, not sure about template lookup), but not "container.hpp"+"number.hpp", which would be correct.

The reason I didn't fix this is for one simply an oversight at the start, but now it is the inability to compile the sdk for various reasons. I have started a separate branch in which I'm working on emitting a freestanding compilable sdk, in which I'm also addressing this bug, see #59

EDIT: I can cherry pick the changes if they turn out to be necessary for this PR. I don't know how users are currently using the generated SDK and how much trouble this bug causes. EDIT2: done

@Cre3per
Copy link
Author

Cre3per commented Aug 23, 2024

This PR might introduce bugs. I can't tell, because the generated SDK doesn't compile, as it didn't compile before this PR either. Any bugs introduced by this PR have already been fixed in #59, which asserts types sizes and field offsets in the generated sdk. Nonetheless, it'd be good to hear from a user if this PR adds regressions. #59 doesn't change the sdk structure, so efforts required for updating a cheat/tool/mod to test this PR are not in vain.

@Cre3per Cre3per marked this pull request as ready for review August 23, 2024 18:09
@Cre3per Cre3per mentioned this pull request Aug 27, 2024
Copy link
Collaborator

@es3n1n es3n1n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick look and made it working on windows, here are a few notes about my changes:

  • On Windows, you can't have : in file names, so specifically for Windows, we are now replacing those with _. Not a big deal but perhaps we may want to have the same behaviour on linux to avoid inconsistencies between platforms?
  • I also added the type name escaping when starting the new struct because such a declaration (class C_BaseFlex::Emphasized_Phoneme {) is ill-formed plus we are using the sanitized name when using these structs as fields. I am not sure if this falls within the scope of this PR as I didn't check Compile sdk #62 yet, although I strongly advise to leave it like this for now.

@Cre3per
Copy link
Author

Cre3per commented Aug 30, 2024

Took a quick look and made it working on windows, here are a few notes about my changes:

  • On Windows, you can't have : in file names, so specifically for Windows, we are now replacing those with _. Not a big deal but perhaps we may want to have the same behaviour on linux to avoid inconsistencies between platforms?
  • I also added the type name escaping when starting the new struct because such a declaration (class C_BaseFlex::Emphasized_Phoneme {) is ill-formed plus we are using the sanitized name when using these structs as fields. I am not sure if this falls within the scope of this PR as I didn't check Compile sdk #62 yet, although I strongly advise to leave it like this for now.

Thank you for your changes. I don't have Windows, so your input is very valuable for this PR!

I've updated EscapePath to do the same on all platforms. The less differences we have between platforms the easier it is to write cross-platform tools using the sdk.

The :: type naming is already fixed in #62. We can keep your fix and I'll merge it with #62 when this PR is done.

Copy link
Collaborator

@es3n1n es3n1n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're good then

@es3n1n es3n1n merged commit fcc2d98 into neverlosecc:main Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants