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

EntityStrongWargDeathBeams #1585

Merged
merged 11 commits into from
Sep 6, 2024

Conversation

Max-Clark
Copy link
Contributor

Thanks again @joshlory @bismurphy for the help!

@joshlory
Copy link
Contributor

joshlory commented Sep 6, 2024

Congrats on the match! 🎊

prim->clut = palette;
prim->u0 = prim->u1 = (temp_s1 & 1) << 7 | 0x21;
prim->v1 = prim->v3 = (temp_s1 & 2) << 6 | 0x59;
prim->v0 = prim->v2 = (temp_s1 & 2) << 6 | 0x7F;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these still match if you use + instead of | at the end? Since UV coordinates are numerical values, it probably makes more sense to add a physical offset than to bitwise OR it.

while (prim != NULL) {
if (prim->drawMode == DRAW_HIDE) {
if (self->ext.factory.unk7E & 1) {
PlaySfxPositional(0x655);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For SFX calls, we usually use the enums in include/sfx.h. In this case, this should be SFX_EXPLODE_B.

prim->priority = self->zPriority + priorityOffset;
prim->drawMode = DRAW_TPAGE2 | DRAW_TPAGE | DRAW_COLORS |
DRAW_UNK02 | DRAW_TRANSP;
prim->p1 = (Random(priorityOffset) & 3) + 0x10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this Random call shouldn't have an argument. The compiler is just reusing the a0 register, I expect. Try removing priorityOffset from this line and see if it still matches.

}

self->ext.timer.t = 4;
self->ext.factory.unk7E++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll probably want to make a new entry in the Ext union in include/entity.h for this entity. Feel free to ask in discord if you don't know how to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a shot at it in a33aa1c, LMK if it needs changed and I'm happy to do so

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good, but I'm not sure why you named the variable zPriority. It looks like it's controlling multiple things, including the zPriority. I would recommend keeping it as unk7E unless we know what it is.

Also, I would recommend adding an unk7C, and using that instead of ext.timer.t. When possible, we want an entity to exclusively use its own Ext member, instead of mixing around self->ext.this and self->ext.that.

prim->y0 = prim->y1 = prim->y1 - prim->p1;

if (prim->p2 > 0x10) {
prim->drawMode = 8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

drawMode uses enums, in this case 8 should be DRAW_HIDE.

@bismurphy
Copy link
Collaborator

Looking like a fantastic first PR, well done! Some cleanup comments, but nothing huge. Looking forward to merging this one!

@Xeeynamo
Copy link
Owner

Xeeynamo commented Sep 6, 2024

Congrats @Max-Clark for your first pull request to the project 👏 .

We are currently transitioning to apply a license to the project. Before merging this pull request we will need you to accept the license here: #1565

@bismurphy bismurphy merged commit ed1689d into Xeeynamo:master Sep 6, 2024
15 checks passed
@Max-Clark Max-Clark deleted the EntityStrongWargDeathBeams branch September 7, 2024 02:44
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.

5 participants