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

Define relationship between map_object and object_events #1054

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

vulcandth
Copy link
Collaborator

Opening this PR to further demonstrate what I'm suggesting should be done in issue #1031. Please review and look it over. If you don't think this is a good idea, or feel we want to have more conversation before this; feel free to close this PR or draft it.

Resolves #1031

@mid-kid
Copy link
Member

mid-kid commented May 31, 2023

If you're moving OBJECT_EVENT_SIZE to be more closely related to MAPOBJECT_*, I'd name it something more apt like MAPOBJECT_STRUCT_LENGTH (we don't have a standard naming scheme for struct sizes...), and move all the other script command macros to their respective structure definitions, too, creating them where missing.

@vulcandth vulcandth marked this pull request as draft May 31, 2023 23:04
Copy link
Member

@Rangi42 Rangi42 left a comment

Choose a reason for hiding this comment

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

Hmm. I liked keeping all the *OBJECT_LENGTH constants together. Although object_event fields do happen to correspond directly to a subset of map_object fields. How about changing script_constants.asm:

; An object_event is a map_object without its leading MAPOBJECT_OBJECT_STRUCT_ID
DEF OBJECT_EVENT_SIZE EQU MAPOBJECT_LENGTH - 1 ; 13

@Rangi42
Copy link
Member

Rangi42 commented Jun 22, 2023

This is also an opportunity to define field constants for all of the warp/coord/bg/object event macros, instead of hard-coding their total sizes.

@vulcandth
Copy link
Collaborator Author

vulcandth commented Jun 23, 2023

Hmm. I liked keeping all the *OBJECT_LENGTH constants together. Although object_event fields do happen to correspond directly to a subset of map_object fields. How about changing script_constants.asm:

; An object_event is a map_object without its leading MAPOBJECT_OBJECT_STRUCT_ID
DEF OBJECT_EVENT_SIZE EQU MAPOBJECT_LENGTH - 1 ; 13

Unfortunately It is without the leading MAPOBJECT_OBJECT_STRUCT_ID and the last two unused bytes at the end of the struct. (See the rb_skip 2).

So it would have to be something like:

DEF OBJECT_EVENT_SIZE EQU MAPOBJECT_LENGTH - 3 ; 13

We could just define it as it's own struct... but... seems kinda redundant.

@Rangi42
Copy link
Member

Rangi42 commented Jun 24, 2023

I'm okay with this:

; An object_event is a map_object without its initial MAPOBJECT_OBJECT_STRUCT_ID or final padding
DEF OBJECT_EVENT_SIZE EQU MAPOBJECT_LENGTH - 3 ; 13

@vulcandth vulcandth marked this pull request as ready for review August 10, 2023 22:36
@vulcandth vulcandth merged commit e07a1e7 into pret:master Aug 14, 2023
1 check passed
@vulcandth vulcandth deleted the object_event_relate branch August 14, 2023 14:19
Idain pushed a commit to Idain/pokecrystal that referenced this pull request Sep 11, 2023
Idain pushed a commit to Idain/pokecrystal that referenced this pull request Sep 12, 2023
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.

Further define relationship between the map_object struct and object_events
3 participants