Remove global dependencies (1) #1804
Conversation
There was a problem hiding this comment.
Something to take in consideration that we are trying to get everybody who has contributed / wants to contribute to MM decomp to agree to licensing the repo before merging PRs from people who has not yet agreed to it. It is better explained here: louist103/mm-licensing#1.
Could you please read about it and post in that issue if you agree or not. Thanks!
| #include "z64lib.h" | ||
| #include "z64play.h" | ||
|
|
||
| #include "global.h" |
There was a problem hiding this comment.
It feels kinda dumb adding all those individual headers and then add global.h too.
Why not just leave alone those files that import gameplay_keep.h up until we have the new assets system?
There was a problem hiding this comment.
i explain why it's not dumb in another comment.
As for the question, it's mainly just that I started to take the same approach to removing global.h as I did with OoT, and realizing a bit late that gameplay_keep.h has more specialized dependencies than in OoT. I can try to avoid it in future PRs.
| // Corresponds to GreatFairyType | ||
| typedef enum ElfgrpType { | ||
| /* 0 */ ENELFGRP_TYPE_MAGIC = GREAT_FAIRY_TYPE_MAGIC, | ||
| /* 1 */ ENELFGRP_TYPE_POWER = GREAT_FAIRY_TYPE_POWER, | ||
| /* 2 */ ENELFGRP_TYPE_WISDOM = GREAT_FAIRY_TYPE_WISDOM, | ||
| /* 3 */ ENELFGRP_TYPE_COURAGE = GREAT_FAIRY_TYPE_COURAGE, | ||
| /* 4 */ ENELFGRP_TYPE_KINDNESS = GREAT_FAIRY_TYPE_KINDNESS, | ||
| /* 0 */ ENELFGRP_TYPE_MAGIC, | ||
| /* 1 */ ENELFGRP_TYPE_POWER, | ||
| /* 2 */ ENELFGRP_TYPE_WISDOM, | ||
| /* 3 */ ENELFGRP_TYPE_COURAGE, | ||
| /* 4 */ ENELFGRP_TYPE_KINDNESS, | ||
| /* 5 */ ENELFGRP_TYPE_MAX | ||
| } ElfgrpType; |
There was a problem hiding this comment.
This is pretty much a regression IMO.
I don't think having the stuff from object_dy_obj visible to z_en_elfgrp and z_en_elforg. You pretty much need to know the correct object for each actor, and the symbols being available won't change that.
There was a problem hiding this comment.
but it's a very minor regression and an overall net positive because now z_en_elfgrp and z_en_elforg are no longer being coupled to an unrelated actor.
Arguably, something like the GreatFairyType enum should be somewhere more global, z64save.h perhaps
There was a problem hiding this comment.
That's a fair point.
I guess we could move the enum to another header and even just reuse the same enum for the 3 actors, but that feels a bit out of scope for this PR.
| #include "z_en_item00.h" | ||
| #include "overlays/actors/ovl_En_Elf/z_en_elf.h" | ||
| #include "overlays/actors/ovl_En_Elforg/z_en_elforg.h" | ||
|
|
||
| #include "libc64/qrand.h" |
There was a problem hiding this comment.
Could you move those overlays includes to the bottom instead?
Overall the include order and grouping we have been following is more or less the following:
Current file header
[Libraries] (ultralib.h, libc, libc64.h, libu64.h, attributes.h, etc)
["Game engine" headers] (gfx.h, gamealloc.h, sys*.h, z64*.h, etc)
[Other overlays]
[Objects]
Could you adjust your PR to follow this order?
There was a problem hiding this comment.
idk the more I do this the more convinced I am at just sorting everything by folder, alphabetical order. Why is attributes.h a "libraries" header? Are there more "libraries" headers hiding in the include folder?
There was a problem hiding this comment.
Well, that's not how we have been sorting includes, and neither is how we intend to do it, so please change them. For example z64.h is more or less sorted this way.
attributes.h is kinda a "libraries" header because it doesn't really fit any other category
| #include "libc64/qrand.h" | ||
| #include "gfx.h" | ||
| #include "gfx_setupdl.h" | ||
| #include "ichain.h" | ||
| #include "macros.h" | ||
| #include "sfx.h" | ||
| #include "sys_matrix.h" | ||
| #include "z64play.h" |
There was a problem hiding this comment.
Changes like this are weird because z_en_butte.h still includes global.h, so this currently doesn't make much sense
There was a problem hiding this comment.
but global.h is not a real header, as in we want to get rid of it, correct? Adding these necessary dependencies here helps reduce our overall dependency on the global.h header, which in turn makes it easier to make cuts to z64.h down the road with less mess.
There was a problem hiding this comment.
Yes, we want to get rid of it, but adding extra includes without actually removing global.h (which internally still includes those headers) does pretty much nothing besides making it annoying for review.
I think it is better to not mess with those files that still have a hard requirement on global.h until other stuff gets fixed.
| #include "sys_matrix.h" | ||
| #include "z64play.h" | ||
|
|
||
| #include "global.h" |
There was a problem hiding this comment.
Why is global.h required here? I don't see any assets
There was a problem hiding this comment.
iirc it depends on GetItem_Draw
There was a problem hiding this comment.
Why not include functions.h explicitly then?
| #include "assets/objects/object_cow/object_cow.h" | ||
| #include "assets/objects/object_dog/object_dog.h" | ||
| #include "assets/objects/object_ma1/object_ma1.h" | ||
| #include "assets/objects/object_ma2/object_ma2.h" | ||
| #include "assets/objects/object_uch/object_uch.h" |
There was a problem hiding this comment.
What's the point of including those objects both in the header and in the C file?
There was a problem hiding this comment.
to be more explicit in expressing the dependencies of the C file.
There was a problem hiding this comment.
In that case why not also include all the headers included by the actor's header? Like, let's include z64actor.h here too!
But no, don't do that, it just increases the maintainability burden.
And what should people be doing on a modding scenario? Add new objects headers to both the actor and the header?
There was a problem hiding this comment.
In a modding scenario, people can do whatever the hell they want with the code. There is no requirement for anyone using decomp to follow our standards.
But anyway, the general philosophy I've been using when it comes to including headers is to try to explicitly include all of the headers required by a given source file (with ultra64 being a bit of an exception). This has the effect keeping files decoupled from the headers they include. It also makes it easier to track what parts of code are dependent on a given header.
In practice, since I've been doing this manually by hand I haven't bothered with any dependencies that come with z64play.h, and might have missed things here and there.
So let's look at z_en_invadepoh.h for example, and pretend global.h isn't there. I should have added either PR/ultratypes.h or ultra64.h, and then z64math.h because there are s8s and Vec3fs. Now, unk.h is interesting, because when you look into z64actor.h it already includes unk.h. But it doesn't make sense to assume that z64actor.h provides that header, and should the unknowns in z64actor.h become known that header is going to be removed. So that's why being explicit with headers is important for maintainability. On the flip side, z_en_invadepoh.h just doesn't require all of the headers z64actor.h is dependent on; stdbool.h, color.h, libu64/pad.h, z64actor_dlftbls.h, z64item.h I believe aren't used so they shouldn't be included into z_en_invadepoh.h. Looking at it, I guess that means that z64animation.h and z64collision_check.h should be included as well. I guess I've been a bit lazy on going deeper into z64actor dependencies too.
AngheloAlf
added
Clean up
Needs-second-approval
|
Changes made in this pr are licensed under CC0. |
|
There are some parts of this PR that still need a bit more discussion, but I think the new |
|
About the current PR, as you have probably noted there are still a few stuff that haven't been moved to a better place and stuff still depend on I think it would be easier to do cleanup in other headers and get rid of other stuff, like, for example, getting rid of |
One thing of note: a pair of actor headers written in a way that made them dependent on
GreatFairyTypeenumz_bg_dy_yoseizo.h. I rewrote them to not require this dependency becausez_bg_dy_yoseizo.hitself imports the dependency ofobject_dy_obj.h, which isn't really valid for either actor.