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

Add bhvDefaultStarMarker and related objects to replace hardcoded star spawn coordinates #830

Open
wants to merge 2 commits into
base: develop/3.0.0
Choose a base branch
from

Conversation

gheskett
Copy link
Collaborator

spawn_default_star and cur_obj_spawn_star_at_y_offset now look for the nearest bhvDefaultStarMarker object with a matching bparam1, rather than using hardcoded coordinates. If no star marker object is found, the star spawn will fall back to the object's home coordinates.

This PR is the sequel to #829.

…r spawn coordinates

spawn_default_star and cur_obj_spawn_star_at_y_offset no longer take coordinates. If no star marker object is found, the star spawn will fall back to the object's home coordinates.
@gheskett gheskett added the enhancement New feature or request label Aug 30, 2024
@gheskett gheskett added this to the 3.0 milestone Aug 30, 2024
@gheskett gheskett requested a review from arthurtilly August 30, 2024 02:18
@gheskett gheskett self-assigned this Aug 30, 2024
@gheskett gheskett mentioned this pull request Aug 30, 2024
@gheskett
Copy link
Collaborator Author

gheskett commented Nov 5, 2024

Will forcefully merge if this is not reviewed before the end of the week.

OBJECT_WITH_ACTS(/*model*/ MODEL_NONE, /*pos*/ -1100, -2372, 1100, /*angle*/ 0, 135, 0, /*behParam*/ 0x01000000, /*beh*/ bhvMerryGoRoundBooManager, /*acts*/ ACT_2 | ACT_3 | ACT_4 | ACT_5 | ACT_6),
OBJECT_WITH_ACTS(/*model*/ MODEL_BOO, /*pos*/ 1030, 1922, 2546, /*angle*/ 0, -90, 0, /*behParam*/ 0x04000000, /*beh*/ bhvBalconyBigBoo, /*acts*/ ALL_ACTS),
OBJECT_WITH_ACTS(/*model*/ MODEL_BOO, /*pos*/ 581, 1850, -206, /*angle*/ 0, -90, 0, /*behParam*/ 0x00000000, /*beh*/ bhvBoo, /*acts*/ ALL_ACTS),
OBJECT_WITH_ACTS(/*model*/ MODEL_BOO, /*pos*/ 1000, 50, 1000, /*angle*/ 0, 0, 0, /*behParam*/ 0x00000000, /*beh*/ bhvGhostHuntBigBoo, /*acts*/ ACT_1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these formatting changes needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Less ugly I guess, though this section also introduces star markers so it makes no real difference in terms of conflicts.

void spawn_default_star(void) {
f32 dist;
struct Object *starObj;
struct Object *starMarker = cur_obj_find_nearest_object_with_behavior_and_bparams(bhvDefaultStarMarker,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is regression compared to vanilla logic where I could specify arbitrary location for a star to spawn. Now if I have multiple spawners, closest will be chosen.

Copy link
Collaborator Author

@gheskett gheskett Nov 8, 2024

Choose a reason for hiding this comment

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

This would only be a problem if you have multiple spawners for the same star ID. Otherwise you'd only be able to have one spawner per area (which would be a problem for obvious reasons).

You can always make a new function for this if you have a use case of overriding the spawn marker, but in general hardcoding star coords is worse in almost every other way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If developer has to change code, this PR does nothing good. If you want to go this way, introduce a enum for bparam2 to search for the object based on that bparam2 instead of nearest object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The exact idea here is that they don't. Having to hardcode coordinates for stars means people can't handle that through fast64 alone, and forgetting to change those coords will result in broken spawns almost every time.

All the developer needs to do is add a star marker object via fast64 to set the spawn location, with a matching bparam1. If no star marker is found, it falls back to the object's home as a safeguard (which is still an improvement over hardcoding).

Comparing against bparam2 as an additional identifier isn't a good option here because that bparam2 is inhereted from the parent object (which could be set to anything). In my opinion though the need for this is an edge case that vanilla doesn't do natively anywhere either. If it's really necessary to keep a hardcoded spawn function in HackerSM64, that too can be added to this PR.

Copy link
Collaborator

@arthurtilly arthurtilly left a comment

Choose a reason for hiding this comment

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

I am not convinced this is a good PR at all and I don't think I ever approved of the concept when it was being discussed. Forgot there was even a PR to begin with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants