agl icon indicating copy to clipboard operation
agl copied to clipboard

Add all headers from OdysseyReversed

Open MonsterDruide1 opened this issue 3 years ago • 2 comments

This PR adds all existing header files from the OdysseyReversed project by shibbo (https://github.com/shibbo/OdysseyReversed/tree/master/include/agl).


This change is Reviewable

MonsterDruide1 avatar Dec 17 '21 14:12 MonsterDruide1

Before merging, the necessary changes in sead have to be done, and src/stub.cpp has to be removed. This file was only added to include all headers, to point out compiler issues.

MonsterDruide1 avatar Dec 17 '21 14:12 MonsterDruide1

Only skimmed through this as this is a rather large PR and I don't really have time to give this a proper review today, but can you add line breaks after namespace xxx { and before } // namespace xxx? e.g.

namespace xxx {

// stuff
// more stuff

}  // namespace xxx

so that things look less cramped.

Other things to fix:

  • function parameters should be named whenever possible. Now obviously I understand that not every function has been properly RE'd but sometimes it's quite obvious what a parameter is. Having named parameters makes things nicer for the person who has to implement those functions (and for autocompletion/docs)
  • virtual functions that are overridden should be marked override (or in some cases final), not virtual. This applies to virtual destructors as well
  • do not use typedefs, use C++11 type aliases (using Foo = Bar;)
  • most of the u64 usages look quite suspicious. Those u64s should probably either be pointers (use void* if you have no idea what the pointer type is) or size_t.

leoetlino avatar Aug 15 '22 21:08 leoetlino

Adjusted most of the files according to the comments, function parameters still missing in a lot of places.

Needs https://github.com/open-ead/sead/pull/119 to be merged first, fixing two signatures of virtual functions.

MonsterDruide1 avatar Jan 21 '23 23:01 MonsterDruide1

Which/How much more changes are required here? Should I maybe split up this PR into multiple parts, so each one can be reviewed/handled individually?

MonsterDruide1 avatar Feb 17 '23 20:02 MonsterDruide1

Can you go ahead and exclude the stub.cpp from the commits? Also is there anything else that needs to be addressed?

ThePixelGamer avatar Feb 22 '23 00:02 ThePixelGamer

detail/aglPrivateResource.h still uses the old hpp include, can you push a fix?

ThePixelGamer avatar Feb 22 '23 16:02 ThePixelGamer

LGTM I'll :shipit: whenever you give the go ahead @MonsterDruide1

ThePixelGamer avatar Feb 23 '23 07:02 ThePixelGamer

Alright, then - go!

MonsterDruide1 avatar Feb 24 '23 20:02 MonsterDruide1