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

Map/object/event renaming #1076

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vulcandth
Copy link
Collaborator

@vulcandth vulcandth commented Aug 22, 2023

Resolves: #1032, #655 

This is what I ended up going with:

Original Labels Recommended Labels
object_struct (current name is fine)
wObjectStructs wObjects
wPlayerStruct / wPlayerSprite wPlayerObject/wPlayerSprite
wObject#Struct / wObject#Sprite wObject#/wObject#Sprite
NUM_OBJECT_STRUCTS NUM_OBJECTS
OBJECT_LENGTH (current name is fine)
Original Labels Recommended Labels
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.

@mid-kid
Copy link
Member

mid-kid commented Aug 23, 2023

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_structs, wObjectEvents contains object_event_struct, etc.

@vulcandth
Copy link
Collaborator Author

vulcandth commented Aug 23, 2023

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.

@vulcandth vulcandth linked an issue Aug 25, 2023 that may be closed by this pull request
4 tasks
@mid-kid
Copy link
Member

mid-kid commented Aug 26, 2023

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

@Rangi42
Copy link
Member

Rangi42 commented Nov 19, 2023

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.

@mid-kid
Copy link
Member

mid-kid commented Nov 20, 2023

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_object_struct, to avoid colliding with SPRITE_ constants)

@vulcandth
Copy link
Collaborator Author

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.

@mid-kid
Copy link
Member

mid-kid commented Nov 20, 2023

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.

@vulcandth
Copy link
Collaborator Author

vulcandth commented Nov 20, 2023

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.

@vulcandth
Copy link
Collaborator Author

vulcandth commented Nov 20, 2023

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.

@mid-kid
Copy link
Member

mid-kid commented Nov 21, 2023

oh, I see. In that case yeah, object may be renamed to something more specific.

@vulcandth
Copy link
Collaborator Author

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.

@mid-kid
Copy link
Member

mid-kid commented Dec 5, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map/object/event renaming Map/object/event cleanup
3 participants