Skip to content

Add methods for CreakingHeart #12095

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Doc94
Copy link
Contributor

@Doc94 Doc94 commented Feb 10, 2025

Draft because need test if call the correct things...

@Doc94 Doc94 marked this pull request as ready for review February 10, 2025 13:05
@Doc94 Doc94 requested a review from a team as a code owner February 10, 2025 13:05
Copy link
Member

@notTamion notTamion left a comment

Choose a reason for hiding this comment

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

some grammar in the jd needs to be fixed

Copy link
Contributor

@Lulu13022002 Lulu13022002 left a comment

Choose a reason for hiding this comment

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

You should also handle unplaced block state properly

tag.putUUID("creaking", this.creakingInfo.map(Entity::getUUID, uuid -> (UUID)uuid));
}
- }
+ tag.putInt(PAPER_CREAKING_MAX_DISTANCE_TAG, this.distanceCreakingTooFar); // Paper - Custom max distance for Creaking
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit rough here if we wanna write.
We should keep the int but we'd ideal also want a way for the developer to fall back to vanillas value even if it changes across versions.

The current implementation would lock every creaking heart at the current vanilla value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe just a
if (this.distanceCreakingTooFar != DISTANCE_CREAKING_TOO_FAR) tag.putInt(PAPER_CREAKING_MAX_DISTANCE_TAG, this.distanceCreakingTooFar); // Paper - Custom max distance for Creaking ?

*
* @return the max distance
*/
int getMaxDistanceForCreaking();
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like this method name.
Especially when we also might add API for the creaking spawn distance.

getProtectorSurvivalRange
setProtectorSurvivalRange
getProtectorSpawnRange
setProtectorSpawnRange
might work, more peoples input tho pls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the lasts ones not like just becauuse that value its not for spawn.. its for limit the max distance where a creaking can "live"

maybe the first can work.. but not sure..

Copy link
Contributor

Choose a reason for hiding this comment

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

The last ones were for the new API regarding the replacement for scaling, sorry xD
But yea, more people time chime in.

Copy link
Member

Choose a reason for hiding this comment

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

getCreakingRemovalDistance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getCreakingRemovalDistance?

not sounds bad... maybe can use that then (based in not another comments xd)

@Doc94 Doc94 requested review from lynxplay and notTamion April 21, 2025 18:41
@notTamion
Copy link
Member

There is a lot of stuff depending on the despawn distance being 34. e.g. Creaking roaming distance and follow distance, redstone output, player spawning range. When setting the despawn distance to anything lower it will cause semi weird behaivour as the creaking will willingly move outside its despawn distance and etc. So i think we should either scale that stuff, Math.min it or add extra api for it. though leaving it half broken like this feels bad

@Doc94
Copy link
Contributor Author

Doc94 commented May 3, 2025

There is a lot of stuff depending on the despawn distance being 34. e.g. Creaking roaming distance and follow distance, redstone output, player spawning range. When setting the despawn distance to anything lower it will cause semi weird behaivour as the creaking will willingly move outside its despawn distance and etc. So i think we should either scale that stuff, Math.min it or add extra api for it. though leaving it half broken like this feels bad

ok for that then im wait for some more feedback about what do...

  • Add a min like Math.min(Math.abs(DISTANCE_CREAKING_TOO_FAR - 2), PLAYER_DETECTION_RANGE) where DISTANCE_CREAKING_TOO_FAR its the custom field
  • Add tags for the others limits and make the user handle that if its going to modify any for not restrict that behaviours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: PR Party candidate
Development

Successfully merging this pull request may close these issues.

6 participants