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

GrS docs for my fork of Pedestal Crafting #30

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

Conversation

MasterEnderman
Copy link
Contributor

Greetings! 👋🏻
It's been a while since I worked with GroovyScript. Any feedback is greatly appreciated.
Link to my fork: https://legacy.curseforge.com/minecraft/mc-mods/pedestal-crafting-patched

Copy link
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

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

checking the source code, have a few comments that are more about that that the documentation

  • PedestalRecipe#initParticles and PedestalRecipe#addDefaultParticles are already called in PedestalRecipe<init>, so probably arent needed
  • the warning message ticks must be greater or equal 5, yet it was {} should probably be greater than or equal to 5
  • probably want to change out the scala use of Int.MaxValue() to Integer.MAX_VALUE and the groovy use of @Nullable to use the jetbrains (or javax?) version.

@WaitingIdly WaitingIdly added the part: groovyscript Related to the GroovyScript documentation label Jan 5, 2025
Copy link
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

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

checking the source code, PedestalRecipe#initParticles and PedestalRecipe#setParticles are called in such a manner that it generates an odd output and means that the default particles are never used.

Copy link
Collaborator

@WaitingIdly WaitingIdly left a comment

Choose a reason for hiding this comment

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

just want to confirm that it is true that the recipe actually needs 1+ items on the surrounding pedestals - depending on the implementation, it might not be required.

@MasterEnderman
Copy link
Contributor Author

just want to confirm that it is true that the recipe actually needs 1+ items on the surrounding pedestals - depending on the implementation, it might not be required.

with it's current implementation it has to be +1 item, but I'll take this as a request to make it work without any input on the outer ring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part: groovyscript Related to the GroovyScript documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants