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

Aversion Obelisk backport to 1.20.1 #777

Merged
merged 16 commits into from
Oct 1, 2024

Conversation

krxdev-kaan
Copy link
Contributor

@krxdev-kaan krxdev-kaan commented Aug 20, 2024

Description

Obelisk Manager system (required for any obelisk) is backported in accordance with 1.21. ChunkBoundLookup (required for the manager system) was backported/imported as is since it did not include anything conflicted between 1.20.1 and 1.21. Aversion Obelisk backported, without Entity Filter support since it isn't present in Ender IO in 1.20.1. Everything was implemented in harmony with the whole structure of Ender IO's implementation. The backport required many things to be changed to their 1.20.1 counterparts, such as: Data Attachment -> Capability, and Generic Network Slot Usage -> (whatever required) Network Slot Usage.

Additional Note: I implemented a new GUI Widget called MessageWidget, to render a custom tooltip/message on any desired area of a GUI without needing a placeholder element rendered. This was implemented to render "This part is a work in progress." message on the Entity Filter slot.

EDIT: Entity Filters were not implemented when I first opened this pull request because I wasn't sure if it was bound to changes even in 1.21 but I have decided to backport it also because it is really useful for Aversion Obelisk and it felt like an unfinished business. If it ever recieves any change, assuming I see it, I can just open another pull request to adjust it to match 1.21 behaviour.

TODO (Maybe)

  • The Aversion Obelisk does not support entity filters as of now, because Entity Filter Item is not present in Ender IO 1.20.1. I did not plan on implementing this since it's even controversial in 1.21 (doesn't have an item slot in the GUI) but if it is requested, I can implement it and write support for it in a couple of days.

Breaking Changes

  • No behaviour has changed but IFilterCapability has been changed to not extend from Predicate This was done because of a double Predicate extend caused by individual capabilities extending from Predicate along with IFilterCapability. This was not a problem for Item and Fluid filters since they did not have 2 different test() methods. This change has literally no effect on any other filter.

@Rover656 Rover656 added Type-Enhancement New feature or enhancement to existing feature. MC-1.20.1 Area-Parity 1.12.2 missing features Type-Backport labels Aug 20, 2024
@krxdev-kaan
Copy link
Contributor Author

I have decided to also backport Entity Filters even though I was unsure if they were bound to changes in 1.21. So, the whole Aversion Obelisk backport is done. I believe it should be ready for a review :)

@ferriarnus
Copy link
Member

Thank you for the PR, I'll try to look into it soon!

@krxdev-kaan
Copy link
Contributor Author

Cool, let me know if you need any changes!

@krxdev-kaan
Copy link
Contributor Author

Hey, I have realized that Rover656 has some updates for 1.20.1 which involved an update on FilterCapability which I was deriving from for the EntityFilterCapability class. So I pushed a small update to conform to the new changes made :) There should be no merging conflict now and I also once again tested everything just in case too. Just wanted to let you guys know so you won't have to review everything once again.

@krxdev-kaan
Copy link
Contributor Author

I have seen that ferriarnus also has updates for 1.20.1 today which involved updates on the base machine block entirely for performance optimizations. I will also make updates once again to conform with these changes but I will be doing it tomorrow. I will once again comment what updates I've made to conform with the changes tomorrow :)

@Rover656
Copy link
Member

@ferriarnus Just to check, do you have a review of this PR in progress or would you like me to take a look, I'm keen to get this into the next 1.20.1 release :)

@krxdev-kaan thanks for all of your hard work on this!

Copy link
Member

@ferriarnus ferriarnus left a comment

Choose a reason for hiding this comment

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

Some small remarks. I'll try to test it in game soon as well.

@krxdev-kaan
Copy link
Contributor Author

Hey, I was just making some commits to conform with the changes you made yesterday. I just changed the getInventoryLayout method to createInventoryLayout method in that commit. Wanted to note it here just in case :) I will now reply to your remarks in a sec.

@krxdev-kaan
Copy link
Contributor Author

@ferriarnus Just to check, do you have a review of this PR in progress or would you like me to take a look, I'm keen to get this into the next 1.20.1 release :)

@krxdev-kaan thanks for all of your hard work on this!

It is my pleasure!

I just saw your latest commits, which were made an hour ago. I will also merge them in a couple hours. Though there seems to be no merge conflicts so I probably will just merge it and leave it.

@krxdev-kaan
Copy link
Contributor Author

I have merged the latest changes once again, no merge conflicts or any issues arose so I am leaving everything as is. Waiting on the review approvals from now on :)

@krxdev-kaan
Copy link
Contributor Author

I have pushed a fix for a very extreme case to crash the game with Aversion Obelisk. If a server closes at a tick where the next tick would have been a spawn event in the range of an obelisk, the next launch of the server would instantly crash it in the first tick for one time (next launch is successful). I was playing with my build for a month now and I have just came across this. This is also possible in 1.21 even though it is REALLY unlikely to happen. I have pushed a fix and I will most likely PR it to 1.21 also.

Explanation:
Aversion Obelisk handles spawn event only if the spawn event is occuring in the range of the obelisk so we call getAABB() while processing and check if AABB contains the event position. However, if this event is occuring in the very first tick of the server, then the obelisk is not loaded yet. Therefore making it's aabb null at the time. This really is a very extreme case but it happened.

Solution:
I simply added a null check for the getAABB() before using it.
I will PR this also to 1.21 because, even though it is for a single time and very unlikely, it can happen.

@krxdev-kaan
Copy link
Contributor Author

krxdev-kaan commented Oct 1, 2024

I've seen that ferriarnus marked everything as resolved. So I am assuming you guys didn't want me to make a change about the last comment. If there is any changes left you would like me to do, please do tell :)

Edit: I have clicked re-review by mistake while I was checking out the changes I've made to make sure I haven't skipped anything. Sorry!

Copy link
Member

@Rover656 Rover656 left a comment

Choose a reason for hiding this comment

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

This looks good to me, don't worry about the EntityFilterSlot - I think it'll be fine (and when I come to backporting some JEI stuff from 1.21 I can always touch it up.

Thanks a lot for backporting these features!

@Rover656 Rover656 dismissed ferriarnus’s stale review October 1, 2024 20:45

PR approved in Discord

@Rover656 Rover656 merged commit 75bb8b2 into Team-EnderIO:lts/1.20.1 Oct 1, 2024
2 checks passed
@krxdev-kaan
Copy link
Contributor Author

Cool! I have joined the discord with my main account also just now. I am really glad that I could be of help! Hoping to port the other obelisks in a couple weeks as soon as my workload gets a bit better :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Parity 1.12.2 missing features MC-1.20.1 Type-Backport Type-Enhancement New feature or enhancement to existing feature.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants