-
Notifications
You must be signed in to change notification settings - Fork 508
Update sounds.json data generation to match vanilla #5030
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
Conversation
Extend the sound type builder with the missing replace value that is accepted by the vanilla SoundEventRegistrationSerializer.
Add API note to clarify the existence of the category field.
Add codec validation to the sound class record. Add additional documentation comments making the goal of data generation explicit.
Remove the category field of the sound event registration class to be added in data generation, as it is not interpreted by vanilla in the sounds file.
Update codec to match validation with entry builder. Add test class for sound type builder to ensure sound types are build correctly. Add test class for sound type codec to ensure sound types generate and are interpreted to sound event registration classes as expected.
Clarify deprecated behavior of category method.
Update param doc to conform to yarn conventions
Remove see tag that might be more distracting than helpful
Remove unused imports. Remove space indentation.
Apply Spotless to add licenses.
Fix package name of sounds test. Replace hard coded value checks with the already existing default variables.
Add default implementation to deprecated method, so implementation does not need to use it anymore.
modmuss50
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, the tests are really good to have.
| * @deprecated Category is not a field interpreted by vanilla in the sounds file, | ||
| * calling this method will have no effect. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use @Deprecated(forRemoval = true) if we know its tottaly broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems sensible to eventually want to get rid of it. I will add a commit for it.
Add for removal to category property method.
Updates the SoundTypeBuilderImpl to create a codec that is fully interpreted by the vanilla SoundEventRegistrationSerialization, as this is what the soundType tries to provide. The links between the implementation records and their vanilla classes are being made explicit with additional documentation.
For the public API this means that a replace options has been added and the category method of the soundTypeBuilder has been made deprecated, as it wasn't processed by vanilla and thus provides no functionality.
News tests are also added. The class SoundTypeBuilderTest tests if the SoundTypeBuilder actually provides the expected SoundType. The class SoundTypeCodecTest tests if the implementation codec generates json that is interpreted to the expected vanilla objects.
Small additional tweaks that have been made are using the existing default variables in matching places and additional validation in the Entry codec to match what is being checked at the builder.