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

Fixed the enchantment table "ready" event, which has been broken for years. #3120

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

zisis912
Copy link
Contributor

@zisis912 zisis912 commented Jul 26, 2023

What used to happen was that the "ready" event would get emitted when all the level values were above 0, however 0 means that a packet hasnt been sent yet

image
image

@zisis912 zisis912 changed the title Updated expected enchant to be a string instead of a numerical ID Fixed the enchantment table "ready" event, which has been broken for years. Also minor structure change Jul 26, 2023
@u9g
Copy link
Member

u9g commented Jul 26, 2023

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

@zisis912
Copy link
Contributor Author

One thing I am worried about is that the packets for level could be sent before the packets for expected.enchant, resulting in something like

[
  { level: 5, expected: { enchant: 'unbreaking', level: -1 } },
  { level: 11, expected: { enchant: null, level: -1 } },
  { level: 30, expected: { enchant: null, level: -1 } }
]

Does anyone know the order with which the packets get sent? Maybe it would be smarter to make the "ready" event reliant on expected.level, depending on the order of the packets

@zisis912
Copy link
Contributor Author

zisis912 commented Jul 26, 2023

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

i dont know how to do that 😭 I made a commit to my fork and it got auto-added to the pull request

I think it's okay if we keep this as a 2 in 1, just a general enchanting library fix and overhaul to modernise it.

@zisis912
Copy link
Contributor Author

After some stress testing I saw that its possible for the level to update without the expected.level updating, so I made sure to change the condition to check for the latter.

image

@u9g
Copy link
Member

u9g commented Jul 26, 2023

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

i dont know how to do that 😭 I made a commit to my fork and it got auto-added to the pull request

I think it's okay if we keep this as a 2 in 1, just a general enchanting library fix and overhaul to modernise it.

That's exactly the problem, the "overhaul" is a breaking change to anyone who uses this now.

@zisis912
Copy link
Contributor Author

zisis912 commented Jul 26, 2023

Please seperate this pr into two seperate prs, since it has two distinctly different changes.

i dont know how to do that 😭 I made a commit to my fork and it got auto-added to the pull request

I think it's okay if we keep this as a 2 in 1, just a general enchanting library fix and overhaul to modernise it.

That's exactly the problem, the "overhaul" is a breaking change to anyone who uses this now.

JavaScript is a language that requires one to actively update their code to the newest documentation, atleast as far as web development goes. I don't think this would go against that principle.

Also I would like to mention that there is not a single person that used the enchanting library, since it was broken this whole time. Think of this as a new feature getting added, instead of an old one being modified. Enchanting did not really exist in a usable form up to this point.

And the few people that did use it are certainly smart and involved enough to know where the issue lies

@zisis912 zisis912 changed the title Fixed the enchantment table "ready" event, which has been broken for years. Also minor structure change Fixed the enchantment table "ready" event, which has been broken for years. Jul 27, 2023
@rom1504
Copy link
Member

rom1504 commented Jul 29, 2023

1.8.8 enchanting test is failing
can you please fix it ?

@zisis912
Copy link
Contributor Author

zisis912 commented May 22, 2024

can you please merge this and ignore 1.8.8, the enchanting module does not work on ANY major version right now. While we are worrying that 1.8.8 might be broken, every major release in the last few years is broken, and this is a fix for them.

I cant manually do this tweak in each of my projects, pls just merge it

@rom1504
Copy link
Member

rom1504 commented May 22, 2024

We can't merge PR which break the CI or it breaks merging any further PR

If you care about this change then take a few minutes to finish it

@zisis912
Copy link
Contributor Author

@rom1504 should be complete? the failed CI check is for the windowOpen event, it has nothing to do with my PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs triage
Development

Successfully merging this pull request may close these issues.

3 participants