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

Added PlayerEvent.ItemSmithingEvent #1693

Open
wants to merge 11 commits into
base: 1.21.x
Choose a base branch
from

Conversation

MeAlam1
Copy link

@MeAlam1 MeAlam1 commented Nov 18, 2024

This PR allows users to create an event solely related to the Smithing Table, This will allow developers to retrieve information like the Template used, the main item and the addition.

Currently there is no event that triggers when a user crafts something using the smithing table.
With this PR we will have that event which will allow developers to have more freedom when developing features around the Smithing Table.

Adds #1569.

In addition to the new Event i have also created comments for the ItemSmeltedEvent and ItemCraftedEvent including the methods within these classes.

I have also renamed the Parameters inside those events to clear them up.

@CLAassistant
Copy link

CLAassistant commented Nov 18, 2024

CLA assistant check
All committers have signed the CLA.

@neoforged-pr-publishing
Copy link

  • Publish PR to GitHub Packages

Executed
```
Run gradlew genPatches to generate patch-files from the patched sources
Run gradlew applyAllFormatting to automatically format sources
Check correct formatting with gradlew spotlessCheck
```
@MeAlam1
Copy link
Author

MeAlam1 commented Nov 18, 2024

For when anyone reads this PR

I followed the steps in CONTRIBUTING.md

But when i execute

Run gradlew genPatches to generate patch-files from the patched sources
Run gradlew applyAllFormatting to automatically format sources
Check correct formatting with gradlew spotlessCheck

It Modifies more files then what i intend with my PR and it breaks?

@sciwhiz12
Copy link
Member

In your case, you likely:

  1. Updated your local 1.21.x branch and ran the setup task.
  2. Did your changes to the source files under projects/neoforge.
  3. Updated your local 1.21.x branch but did not run the setup task.
  4. Ran genPatches then committed.

What you're seeing is the result of having your source files (which the patch files are based off) not being in sync with your patch files.

Whenever you update your local repository's main branch (1.21.x in this case) to match upstream, the next thing you should always do is to run setup. Never update your main branch without running setup, as otherwise your workspace will contain elements from before and after updating the branch. (And never update the main branch and run setup without first having generated patches beforehand to save your work under projects/neoforge.)

@sciwhiz12
Copy link
Member

To fix the issue, you can either:

  1. Begin anew on a new branch based off 1.21.x, do your changes there, generate patches, commit, and then force-push that new branch to your repository's PR branch such as git push --force origin my-new-branch:PlayerSmithingEvent;
  2. Edit your commits via git rebase -i or git commit --amend to remove the unwanted changes to the patch files, and then immediately afterwards do a setup to sync the patch files to the source code; or
  3. Manually restore the patch files in your local environment to how they are in the current upstream repository, then run setup to sync the patch files to the source code.

@MeAlam1
Copy link
Author

MeAlam1 commented Nov 18, 2024

Thank you for explaining!

I will resolve this issue as soon as possible.

* Resolved the issue
@MeAlam1
Copy link
Author

MeAlam1 commented Nov 18, 2024

I have resolved the issue using:

Manually restore the patch files in your local environment to how they are in the current upstream repository, then run setup to sync the patch files to the source code.

Thank you very much for helping me so quickly.

@sciwhiz12 sciwhiz12 added enhancement New (or improvement to existing) feature or request 1.21.3 Targeted at Minecraft 1.21.3 labels Nov 18, 2024
@sciwhiz12 sciwhiz12 self-requested a review November 29, 2024 20:11
@@ -406,6 +406,37 @@ public Container getInventory() {
}
}

public static class ItemSmithingEvent extends PlayerEvent {
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see some documentation on this class. Most of our events should have a javadoc explaining:

  • What the event's purpose is (e.g. "An event for when a player smiths an item")
  • When the event is fired (e.g. "After awarding used recipes, but before resizing the stacks")
  • Where the event is fired (e.g. on the logical server, client, both)
  • Which bus it's fired on (e.g. the game bus)

Copy link
Author

Choose a reason for hiding this comment

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

Understood, i will see to this asap

Copy link
Author

@MeAlam1 MeAlam1 Dec 8, 2024

Choose a reason for hiding this comment

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

This should have been completely resolved!

I will not close this conversation since i do not want to hide it before it has been confirmed by a Maintainer

* `ItemSmeltedEvent`
* `ItemCraftedEvent`
* ItemSmithingEvent`

Using the Comment styling discussed in #neoforge-github (Discord)

I also Commented the Methods within the Classes and renamed the parameters that were misleading
this.craftMatrix = craftMatrix;
}

/**
* {@return the item that was crafted (ex. Diamond sword)}
*/
public ItemStack getCrafting() {
Copy link
Author

Choose a reason for hiding this comment

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

This Method name, and the same method from the ItemSmeltedEvent are misleading.

I recommend renaming them to getResult or getCrafted(for the ItemSmeltedEvent, getSmelted) since they return the result, and not the matrix/item that is being smelted.

Since this is a Breaking Change i was requested in the Discord to make a comment about it

@MeAlam1
Copy link
Author

MeAlam1 commented Dec 8, 2024

Also, small detail

The Label 1.21.3 should be modified to 1.21.4 since this PR is now Up to Date with the latest commits within the 1.21.x branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21.3 Targeted at Minecraft 1.21.3 enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants