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

Enemy: Implement KuromadoMagicBall #69

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

Conversation

GRAnimated
Copy link
Collaborator

@GRAnimated GRAnimated commented Jun 5, 2024

This is a small unused actor that would have been used by a nonexistent Kuromado actor, also known as the Magikoopa.


This change is Reviewable

Copy link
Owner

@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.

Reviewed 7 of 9 files at r1, all commit messages.
Reviewable status: 7 of 9 files reviewed, 1 unresolved discussion (waiting on @GRAnimated)


lib/al/include/Library/LiveActor/ActorInitFunction.h line 26 at r1 (raw file):

class SceneCameraInfo;

void initActorSceneInfo(LiveActor*, const ActorInitInfo&);

How did you sort these, and what's the rule to split them between ActorInitFunction and ActorInitInfo? If the latter one is an actual file, there should be more functions listed here in that one.

@MonsterDruide1
Copy link
Owner

Also, this will probably conflict with the other, huge PR, so we'll probably get this one sorted out first, then rebase the other - but let's see.

@LynxDev2 LynxDev2 force-pushed the kuromado-magic-ball branch from 4b4748e to 0e133bf Compare October 28, 2024 17:22
Copy link
Contributor

@LynxDev2 LynxDev2 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: 2 of 17 files reviewed, 1 unresolved discussion (waiting on @MonsterDruide1)


lib/al/include/Library/LiveActor/ActorInitFunction.h line 26 at r1 (raw file):

Previously, MonsterDruide1 wrote…

How did you sort these, and what's the rule to split them between ActorInitFunction and ActorInitInfo? If the latter one is an actual file, there should be more functions listed here in that one.

In my opinion ActorInitInfo should only have the class and the al functions should be in ActorInitFunction. I moved all of them there. There are some actors and objects that only include ActorInitInfo for the functions causing this change to break compiling. I don't want to add too much additional file changes to this PR which is why I made ActorInitInfoinclude ActorInitFunction as a temporary solution. If you want me to change all the actor classes instead, let me know

Copy link
Owner

@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.

Reviewed 10 of 15 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 14 of 17 files reviewed, 2 unresolved discussions (waiting on @GRAnimated and @LynxDev2)


lib/agl line 1 at r3 (raw file):

Subproject commit 20374c4ca7a41552f1767756730497374eef368d

Should this be changed? Why is it necessary?


lib/al/include/Library/LiveActor/ActorInitFunction.h line 26 at r1 (raw file):

Previously, LynxDev2 wrote…

In my opinion ActorInitInfo should only have the class and the al functions should be in ActorInitFunction. I moved all of them there. There are some actors and objects that only include ActorInitInfo for the functions causing this change to break compiling. I don't want to add too much additional file changes to this PR which is why I made ActorInitInfoinclude ActorInitFunction as a temporary solution. If you want me to change all the actor classes instead, let me know

If you check the function map, the ActorInitInfo:: functions are somewhere in the middle of those that you just merged/moved into ActorInitFunction. This layout only seems to be re-creatable when using a single file, not splitting them, or am I missing something here?

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