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.

Sign up for GitHub

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

Jump to bottom

SlimefunItemStack reimplementation #4251

Open
wants to merge 53 commits into
base: walshy/mc-1.21
Choose a base branch
from
Open

SlimefunItemStack reimplementation #4251

wants to merge 53 commits into from

Conversation

Intybyte
Copy link
Contributor

@Intybyte Intybyte commented Oct 22, 2024 •
edited
Loading

Description

We can't extend itemstack anymore we need to fix EVERYTHING

FOR PEOPLE DOWNLOADING SF FROM HERE: ALL ADDONS WILL BREAK

Proposed changes

Use a delegate instead of extending itemstack

Related Issues (if applicable)

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them. This is the frontrow for watching all addons burn.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.

@github-actions GitHub Actions
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility
If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@WalshyDev WalshyDev changed the base branch from master to walshy/mc-1.21 October 22, 2024 20:33
@JustAHuman-xD
Copy link
Contributor

Really not a fan of getDelegate tbh, not sure a great other solution tho

@Intybyte
More fixed, most notably ChestMenuUtils
70c96e0 
NOTE TO FUTURE SELF: Use ForwardingObject over lombok's @DeleGate
@Intybyte
More refactoring
e7aefc6
@Intybyte
Copy link
Contributor Author

Intybyte commented Oct 25, 2024 •
edited
Loading

Once this works i will try replacing it with google's ForwardingObject rather than adding a new dependency, similar to how many paper classes are implemented so to not get surprises later on

(Edit) this breaks a lot of compatibility compared to using lombok as you need to first get the delegate and then run the methods on the delegate, instead lombok generates code to access said delegate methods directly

This was referenced Nov 7, 2024
Open
Closed
@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Nov 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 9, 2024 •
edited
Loading

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 768a3d2

https://preview-builds.walshy.dev/download/Slimefun/4251/768a3d20

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

Intybyte added 11 commits November 9, 2024 20:54
@Intybyte
Merge branch 'refs/heads/walshy/mc-1.21' into walshy/mc-1.21-sfi
787851c 
# Conflicts:
#	pom.xml
#	src/main/java/io/github/thebusybiscuit/slimefun4/api/gps/GPSNetwork.java
#	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/electric/machines/CarbonPress.java
#	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/electric/machines/ElectricPress.java
#	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/electric/machines/HeatedPressureChamber.java
#	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/electric/reactors/Reactor.java
#	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/magical/talismans/EnderTalisman.java
#	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/magical/talismans/Talisman.java
@Intybyte
Small error on merge
35396d0
@Intybyte
Merge fixes + more fixes
dfa4e94
@Intybyte
GrindStone fix
3435390
@Intybyte
MagicEyeOfEnder fix
3512cd3
@Intybyte
NetherStarReactor fix
af27545
@Intybyte
NuclearReactor fix
1b73f98
@Intybyte
OilPump fix
1f28097
@Intybyte
OreCrusher fix
cc45f7d
@Intybyte
OrganicFertilizer fix
5e25492
@Intybyte
OrganicFood fix
8969397
@Intybyte
Copy link
Contributor Author

Intybyte commented Nov 12, 2024 •
edited
Loading

No idea why it still says Merge Conflicts when i already solved those, it looks like there are some problems, rewriting them here for consistency

  • SoulBound tests don't pass yet
  • Locked items needs a new implementation, their tests are temporarily disabled Now creating defensive copies
  • Run delombok to remove the dependency that I put on SlimefunItemStack

@Intybyte Intybyte marked this pull request as ready for review November 16, 2024 13:38
@Intybyte Intybyte requested a review from a team as a code owner November 16, 2024 13:38
@Boomer-1 Boomer-1 added the Build tested Used to indicate the PR preview build has been tested by one of the team label Nov 18, 2024
@Boomer-1
Copy link

tested this and everything seems to be working fine. no errors in console and all items working as intended.

@Intybyte
Copy link
Contributor Author

WrongItemStackException isn't used anymore, should it be removed altogether? also SlimefunItem#isItemStackImmutable is now useless as I am returning defensive copies, should I remove that one too?

@ybw0014
Copy link
Contributor

ybw0014 commented Nov 22, 2024

WrongItemStackException isn't used anymore, should it be removed altogether? also SlimefunItem#isItemStackImmutable is now useless as I am returning defensive copies, should I remove that one too?

Search for the exception and the function, and the result is only Slimefun core and its fork. No other plugins use them, so I say just remove them.

@ybw0014
Copy link
Contributor

ybw0014 commented Nov 23, 2024

Pull request made: Intybyte#3
or you want to use patch: https://discord.com/channels/565557184348422174/565570276038017044/1309652382279401605

@Boomer-1
Copy link

retested 11-24-24 all seems to be working fine on preview build 6d38511

@Intybyte
Copy link
Contributor Author

No need to retest from 0 the last commit as it just removes unused code

ybw0014
ybw0014 suggested changes Nov 30, 2024
View reviewed changes
src/main/resources/tags/bundles.json Outdated Show resolved Hide resolved
@gurrrrrrett3
Copy link

We've done a ton of testing, had no issues, everything is working perfectly

@JustinDevB
Copy link

What is preventing this from being merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustAHuman-xD JustAHuman-xD JustAHuman-xD requested changes

@ybw0014 ybw0014 ybw0014 requested changes

Assignees
No one assigned
Labels
Build tested Used to indicate the PR preview build has been tested by one of the team ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Intybyte @JustAHuman-xD @Boomer-1 @ybw0014 @gurrrrrrett3 @JustinDevB @TheBusyBot