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

Collector's Bee Ring feature #19

Merged
merged 14 commits into from Apr 30, 2023
Merged

Conversation

Cardinalstars
Copy link

Adding a bauble that simulates a Bee Collector's Jar using NBT stored on the item.

The main logic is located in ItemBeeRing.java and InventoryBeeRing.java. GUI and container are basically copy-pasted from the Collector's Bee Jar. RingHousing.java simulates an apiary just enough to get the effect to trigger.

I'm sorry if it's coded horribly; this is my first major Minecraft feature. :)

@Dream-Master Dream-Master requested review from OneEyeMaker and a team and removed request for OneEyeMaker April 27, 2023 17:58
Copy link

@OneEyeMaker OneEyeMaker left a comment

Choose a reason for hiding this comment

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

Please, reduce amount of copy-pasted code. Either create single Container/UI/logic implementation (for both ring and jar) or create base classes with common details.

And if you need help with coding, you can ask me on Discord (OneEyeMaker#4423) or other devs of GTNH.

src/main/java/magicbees/item/ItemBeeRing.java Outdated Show resolved Hide resolved
src/main/java/magicbees/item/ItemBeeRing.java Outdated Show resolved Hide resolved
src/main/java/magicbees/item/ItemBeeRing.java Outdated Show resolved Hide resolved
src/main/java/magicbees/item/ItemBeeRing.java Outdated Show resolved Hide resolved
src/main/java/magicbees/tileentity/RingHousing.java Outdated Show resolved Hide resolved
Alastors
Alastors previously approved these changes Apr 28, 2023
Copy link
Member

@Alastors Alastors left a comment

Choose a reason for hiding this comment

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

Looks good to me, even with the code comment, I tested it out and it does indeed work

@Dream-Master
Copy link
Member

@OneEyeMaker please can you re check pr.
@Alastors oneEyeMake review it so not approve it until the review is done

@Dream-Master Dream-Master dismissed Alastors’s stale review April 28, 2023 05:43

Already under review

@Alastors
Copy link
Member

@OneEyeMaker please can you re check pr. @Alastors oneEyeMake review it so not approve it until the review is done

Aight, the code itself looked fine and most of his requested changes were done besides a subjective "don't copy and paste code" request, so I thought it'd be fine, the code itself works so.

@Dream-Master
Copy link
Member

@Cardinalstars please run spotlessaply
@Alastors working code is ok but code quality need to be checked as well.
@OneEyeMaker maybe give tips and tricks how Cardinalstars improve the code if necessary

@OneEyeMaker
Copy link

@Alastors I'm sorry, but your changes made code even more messy.
@Cardinalstars After more deep research of this implementation I found performance issue which I don't know how to fix right now. I'll reimplement this idea myself when I find solution. So I close this PR.

@Alastors
Copy link
Member

I think we should just leave this in limbo until someone fixes it instead of closing this out entirely, I don't think just saying "I don't know how to fix this, I'll reimplement this myself eventually, no one else can fix this" is a good enough reason to just nuke a feature pr

@Alastors Alastors reopened this Apr 29, 2023
@Alastors
Copy link
Member

Alastors commented Apr 29, 2023

Either offer a new implementation as a justification to yeet this, or offer solutions to its lone issue, of which multiple have been offered in the discord, such as it only checking like once every minute or such, or as I proposed, only one bee being allowed in it while the effect was happening, and not allowing any more until the bee was destroyed.

@chochem chochem marked this pull request as draft April 29, 2023 01:43
@chochem
Copy link
Member

chochem commented Apr 29, 2023

set to draft by wishes of cardinalstar and alastor

@Alastors
Copy link
Member

Alastors commented Apr 29, 2023

posted that (deleted message) on the wrong pr

@OneEyeMaker
Copy link

OneEyeMaker commented Apr 29, 2023

I apologize for my previous statement. My mind was overclouded yesterday.
Summary of changes:

  • Simplified Container/UI/inventory code by using existing (Forestry) implementation.
  • Fixed crash with particle render on incorrect side.
  • Removed unnecessary setting of inventory slot.
  • Allowed wearing two rings at once with different bees providing different effects.

Probably needs to fix:

  • Ring reads inventory from NBT on every tick. Reducing tick rate N times is not a proper solution - because bee inside ring will work N times slower and live N times longer. And potion effects will update N time more rarely.

(I say "probably", because I haven't large and populated server to test.)

@OneEyeMaker OneEyeMaker marked this pull request as ready for review April 29, 2023 10:33
@OneEyeMaker OneEyeMaker dismissed their stale review April 29, 2023 10:35

I fixed necessary issues myself

…ame. After testing it seems to work, but if somewhere an effect is implemented very weird it might not.
@Dream-Master
Copy link
Member

@Cardinalstars is this ready for review ?

@Cardinalstars
Copy link
Author

@Dream-Master Yes, it should now be ready. I just needed to test on dedicated server.

Copy link
Member

@Alastors Alastors left a comment

Choose a reason for hiding this comment

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

Yeah looks fine to me, my only comment being that this needs extensive testing in large multiplayer worlds (zeta) to ensure we don't need to do more to address the possible lag that nbt checking could be, likewise,it needs a recipe, but things like this can bee handled at a later date.

@Dream-Master
Copy link
Member

@OneEyeMaker

@Dream-Master Dream-Master merged commit e053b01 into GTNewHorizons:master Apr 30, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants