Skip to content

Normalizes CraftEntity#toString/getHandle#12170

Merged
Owen1212055 merged 1 commit into
PaperMC:mainfrom
Lulu13022002:up/entity-tostring
May 1, 2025
Merged

Normalizes CraftEntity#toString/getHandle#12170
Owen1212055 merged 1 commit into
PaperMC:mainfrom
Lulu13022002:up/entity-tostring

Conversation

@Lulu13022002
Copy link
Copy Markdown
Contributor

Instead of hardcoding the class name, fetch it dynamically and show more useful info to distinguish the instances

@Lulu13022002 Lulu13022002 requested a review from a team as a code owner February 23, 2025 20:44
Copy link
Copy Markdown
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, however I am a bit split on what is then actually included in the toString().

The fact that 99% of entities now only expose their uuid is fine to me, I don't think toString() is a reliable
debugging source in the first place.
Noting that, I am confused why you chose to keep CraftHumanEntity's name in the toString impl?
It sticks out a bit given other entities lost their debug information (like horse variants or itemframe rotations).


@Override
public String toString() {
return this.getClass().getSimpleName() + "{uuid=" + this.getUniqueId() + '}';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe can just use handle.getScoreboardName so we don't have to leave the toString implementation in CraftPlayer either.

@Doc94
Copy link
Copy Markdown
Member

Doc94 commented Feb 26, 2025

How hard its make more "dynamic" the getHandle for avoid declare that for every class? or is not related to the point of this PR?

@lynxplay
Copy link
Copy Markdown
Contributor

lynxplay commented Feb 26, 2025

Unrelated to this PR and effectively impossible unless we want to create CraftEntity<E extends nms.Entity> which seems like a lot

@Machine-Maker
Copy link
Copy Markdown
Member

There are probably some entity types that should have extra stuff, like itemstack (the itemstack itself).

Do we want to just do that later, starting from this point?

@Lulu13022002
Copy link
Copy Markdown
Contributor Author

I don't think so, i think the minimal info we show here, the better otherwise it's very arbitrary like before. The only reason i put the player name is because player is a special entity. But i'm fine following lynx advice and just show the uuid for all entities.

@electronicboy
Copy link
Copy Markdown
Member

99% of the time I log some random object, it's because I want some level of information from the thing; I did ponder the idea of using kyori examinables type of setup here so that we can more cleanly control what ends up being dumped out, things like "is this entity valid still" is always nice to see;

Copy link
Copy Markdown
Member

@Owen1212055 Owen1212055 left a comment

Choose a reason for hiding this comment

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

At which point however, that all seems like scope creep.

This looks good to me.

@Owen1212055 Owen1212055 force-pushed the up/entity-tostring branch from 8eeb419 to a372780 Compare May 1, 2025 01:14
@Owen1212055 Owen1212055 merged commit bc3d946 into PaperMC:main May 1, 2025
3 checks passed
@github-project-automation github-project-automation Bot moved this from Awaiting final testing to Merged in Paper PR Queue May 1, 2025
@Lulu13022002 Lulu13022002 deleted the up/entity-tostring branch June 26, 2025 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Merged

Development

Successfully merging this pull request may close these issues.

7 participants