Open vulcandth opened 1 year ago
Reflecting on past changes, I'm not sure overloading "object event" to mean anything other than "a map object's event" was a good idea. However, we went with it, and I prefer consistency over correctness, so these changes are overall fine.
One thing I'll request though is to reserve the term "struct" for the macro names, e.g. wObjects
contains a bunch of object_struct
s, wObjectEvents
contains object_event_struct
, etc.
In a perfect world I would prefer wObjectStructs
=> wMapObjects
and then wMapObjects
=> wMapObjectEvents
... However this would be very confusing for old repo's.... Due to the name re-use.
When I get time i'll drop the "struct" for anything other than the macro's. If anyone thinks of something or comes up with a different naming idea just mention it.
Edit: Updated the original post with mid-kid's suggestions.
wObjects isn't related to maps specifically, it's related to the sprite animation subsystem, so it wouldn't make sense to call it wMapObjects, anyway
I don't see any errors here. There are some quirks like now having a field named OBJECT_OBJECT_EVENT_STRUCT
, or the easily confusible object_event
and object_event_struct
macros. Plus it's yet another widespread naming change. I don't know if the greater consistency here is worth that. I'll defer to @mid-kid since you motivated this in #639 anyway.
I'll be honest, I'm completely out of this effort, and don't fully remember what I intended, so take my ramblings with a grain of salt.
As for object_event and object_event_struct, those still make sense even if they don't look "right" at first glance. One is in rom, the other is in ram, and both directly reflect eachother, they're the same.
I do agree with the struct field constants looking awkward now, but that's an artifact of the map_object->object_event rename. IMO it wasn't the most well thought-out move, but it only makes sense to finish what we started instead of leaving it in this haphazard state. For now I guess we can go with OBJECT_EVENT_INDEX
instead of OBJECT_OBJECT_EVENT_INDEX
. I'd have to read the code of the sprite subsystem again, but if there's any trend towards calling these "Sprites" instead (like we have UpdateSprites
), I'd consider renaming the object_struct to sprite_struct (or sprite_objectstruct, to avoid colliding with SPRITE constants)
I don't personally like calling them "Sprites"... as the game often adds sprites to the screen that don't use these structs. (Pokemon Fly leaves are an example.)... Furthermore; the battle Anim system, party menu sprites, ect.. don't use these structs... only the overworld. However, i'm not going to make a big fuss about it.
We could do ow_object_struct
and OW_OBJECT_EVENT_INDEX
. where ow
stands for Overworld.
I'm just pitching ideas. Think on it.
the game often adds sprites to the screen that don't use these structs
The object structs are the ones that keep the info for the OAMs, are they not? And they're used by menus and the title screen, i.e. anything that isn't battle anims, iirc.
the game often adds sprites to the screen that don't use these structs
The object structs are the ones that keep the info for the OAMs, are they not? And they're used by menus and the title screen, i.e. anything that isn't battle anims, iirc.
I know for a fact that the fly leaf animations write directly to wShadowOAM sprite_anim_struct
bypassing the object structs.. Let me double check the other menus... I may be mistaken.
Yeah; the party menu uses the sprite_anim_struct
. It doesn't use the object_struct
or map_object
/object_event_struct
.
The structs we are talking about in this PR are ONLY used for objects in the overworld.. aka NPC's, ect.
oh, I see. In that case yeah, object may be renamed to something more specific.
Original Labels | Recommended Labels |
---|---|
object_struct |
overworld_obj_struct |
wObjectStructs |
wOverworldObjects |
wPlayerStruct / wPlayerSprite |
wPlayerObject /wPlayerSprite (This is for simplicity) |
wObject#Struct / wObject#Sprite |
wOverworldObj# /wOverworldObject#Sprite |
NUM_OBJECT_STRUCTS |
NUM_OVERWORLD_OBJECTS |
OBJECT_LENGTH |
OVERWORLD_OBJECT_LENGTH |
OBJECT_* /OBJECT_MAP_OBJECT_INDEX |
OVERWORLD_OBJECT_* /OVERWORLD_OBJECT_EVENT_ID |
Original Labels | Recommended Labels |
---|---|
map_object |
overworld_event_struct |
wMapObjects |
wOverworldEvents |
wPlayerObject /wPlayerObjectSprite |
wPlayerEvent /wPlayerEventSprite (This is for simplicity) |
wMap#Object /wMap#ObjectSprite |
wOverworldEvent# / wOverworldEvent#Sprite |
NUM_OBJECTS |
NUM_OVERWORLD_EVENTS |
MAPOBJECT_LENGTH |
OVERWORLD_EVENT_LENGTH |
MAPOBJECT_* /MAPOBJECT_OBJECT_STRUCT_ID |
OVERWORLD_EVENT_* /OVERWORLD_EVENT_OBJECT_ID |
These are new proposed changes... Its a little wordy.. but I prefer being able to easily look at a name and know what it is.. instead of having look it up.. even if it means its a little longer. I don't think this is unreasonable though. Let me know what you think @mid-kid . If you prefer me to update the PR with these changes before you say yay or nay.. I can do that too.
If you shorten a term, like "overworld_obj", be consistent about it, so "OverworldObjs" and "OVERWORLD_OBJ_LENGTH". In this case I'd just spell it out.
As for the "overworld event" term, it sounds awkward and might be confused for tile events which is annoying. I like the idea of going back on calling them "object events" and reserving that term explicitly for the static version of the struct in a map script, but we'd have to make sure the usage of this term is consistent. *MapObject*
-> *ObjectEvent*
was initially proposed in order to align to the new naming in map scripts, and some things were renamed accordingly, which would have to be renamed back.
Resolves: #1032, #655
This is what I ended up going with:
object_struct
wObjectStructs
wObjects
wPlayerStruct
/wPlayerSprite
wPlayerObject
/wPlayerSprite
wObject#Struct
/wObject#Sprite
wObject#
/wObject#Sprite
NUM_OBJECT_STRUCTS
NUM_OBJECTS
OBJECT_LENGTH
map_object
object_event_struct
wMapObjects
wObjectEvents
wPlayerObject
/wPlayerObjectSprite
wPlayerObjectEvent
/wPlayerObjectEventSprite
wMap#Object
/wMap#ObjectSprite
wObjectEvent#
/wObjectEvent#Sprite
NUM_OBJECTS
NUM_OBJECT_EVENTS
MAPOBJECT_LENGTH
OBJECT_EVENT_LENGTH
MAPOBJECT_*
OBJECT_EVENT_*
Anywho; this is just to keep the conversation going; I don't mind reworking this if we decide to do something different.