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

MapObj: Implement ChurchDoor #164

Merged
merged 1 commit into from
Oct 23, 2024
Merged

MapObj: Implement ChurchDoor #164

merged 1 commit into from
Oct 23, 2024

Conversation

MonsterDruide1
Copy link
Owner

@MonsterDruide1 MonsterDruide1 commented Sep 4, 2024

I wanted to do another actor today, so my "randomly scroll around and select one" strategy landed on this one.

Notable things about the actor itself:

  • It only saves whether the door is open or not in "binary" form, there is no intermediate (amount of hits). It is considered "open" after the animation of the third hit has finished playing.
  • This state is only persisted for MoonWorldWeddingRoomStage, meaning the church on the moon itself - not the rematch.
  • Attacks are only counted when the type is "Cappy hits the wall of the door". After the first and second hit, the first 10 frames the door is "immune" to another attack.

Notable things from the programmer's side:

  • The amount of hits (3) and each individual animation is hardcoded, presenting a nerve for both "open" and "close" for every number of hits. The "open" nerve is active while the "hit" action plays, then "close" is entered again.

For other decompilers:
This PR is fully matching. However, one line occurs twice that I'm not quite happy with:
this ? this : nullptr
=> this pattern usually happens when a LiveActor is casted to some of its supertypes using just this. However, in this case, it seems to be required to explicitly write this ? this : nullptr, causing a compiler warning (this should never be null, so this check is "useless"). If anyone finds an alternative, I'm happy to change the two occurences of this!

https://decomp.me/scratch/O4tVr


This change is Reviewable

@MonsterDruide1 MonsterDruide1 self-assigned this Sep 4, 2024
Copy link
Contributor

@Fuzzy2319 Fuzzy2319 left a comment

Choose a reason for hiding this comment

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

I remember something similar in one of my PR. This get solved by an inline function so I tried and get a match like this https://decomp.me/scratch/J82tg

Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: 6 of 7 files reviewed, all discussions resolved (waiting on @MonsterDruide1)

Copy link
Owner Author

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Ooh, great comment! This also helps justify putting the magic value "MoonWorldWeddingRoomStage" into a separate function, to avoid writing it twice in the file. Adjusted accordingly, thanks!

Reviewable status: 6 of 7 files reviewed, all discussions resolved (waiting on @MonsterDruide1)

Copy link
Contributor

@Fuzzy2319 Fuzzy2319 left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @MonsterDruide1)


src/MapObj/ChurchDoor.cpp line 31 at r2 (raw file):

}  // namespace

bool isCurrentStageMoonWeddingRoom(const al::LiveActor* actor) {

I think it should be a header defined function because a function defined only in the cpp file would result in a function without symbol in the binary. However there is no such function near ChurchDoor's functions.

Copy link
Owner Author

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @Fuzzy2319 and @MonsterDruide1)


src/MapObj/ChurchDoor.cpp line 31 at r2 (raw file):

Previously, Fuzzy2319 wrote…

I think it should be a header defined function because a function defined only in the cpp file would result in a function without symbol in the binary. However there is no such function near ChurchDoor's functions.

Moving it to the header requires including a bunch more header files, which I don't really like, but ... I guess that's still the most accurate way to implement this. Done.

Copy link
Contributor

@Fuzzy2319 Fuzzy2319 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MonsterDruide1)

@LynxDev2
Copy link
Contributor

If you mark a function as inline and the compiler doesn't think it's too big to be inlined, the function won't appear in the executable. Using an inline function is way better than implementing isCurrentStageMoonWeddingRoom in a header in my opinion

Copy link
Owner Author

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Good catch! I tested and can confirm that functions declared as inline and only within the cpp fully disappear when compiling. Adjusted accordingly.

Reviewable status: 5 of 7 files reviewed, all discussions resolved (waiting on @Fuzzy2319)

Copy link
Contributor

@Fuzzy2319 Fuzzy2319 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MonsterDruide1)

@MonsterDruide1 MonsterDruide1 merged commit 02e291c into master Oct 23, 2024
11 checks passed
@MonsterDruide1 MonsterDruide1 deleted the church-door branch October 23, 2024 12:49
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.

3 participants