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

fix displayName for items that are also blocks #112

Closed
wants to merge 4 commits into from
Closed

fix displayName for items that are also blocks #112

wants to merge 4 commits into from

Conversation

kaduvert
Copy link

items which are also blocks lack variations in their spec, leading to wrong displayNames

i assume the correct path to fixing this is to check if the item is also a block and use the displayName from the block variation if applicable

@rom1504
Copy link
Member

rom1504 commented Sep 16, 2023

I think we should rather fix the data, maybe using that same heuristic

@kaduvert
Copy link
Author

work smarter, not harder.
it's a 4 line change vs 100s of 1000s of lines of json data

why is this a bad way of fixing the problem?

@kaduvert
Copy link
Author

also there is no downside to this because the display names are already wrong, they can either get right or stay wrong
this only affects names that are wrong already

sorry if i'm too pushy but i really don't get why we should go through all of this effort

@rom1504
Copy link
Member

rom1504 commented Sep 16, 2023

From my point of view having 2 different values for displayName of items throughout PrismarineJs is a strict regression

@rom1504
Copy link
Member

rom1504 commented Sep 16, 2023

Also if you apply it on Minecraft data you will realize this heuristic doesn't work for version above 1.13

@kaduvert
Copy link
Author

kaduvert commented Sep 16, 2023

are there even any wrong displayNames in versions above 1.13?

@kaduvert
Copy link
Author

after thinking for a moment, you're right.
will look into completing the data with the same heuristic and open a pr in minecraft-data.

@kaduvert kaduvert closed this Sep 16, 2023
@kaduvert
Copy link
Author

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.

2 participants