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

api/Open up range field #4231

Merged
merged 9 commits into from
Nov 9, 2024

Conversation

Intybyte
Copy link
Contributor

@Intybyte Intybyte commented Sep 4, 2024

Description

Add new EXP collectors

EXP Collector II: Collects XP orbs in a 7x7x7 area.
EXP Collector III: Collects XP orbs in a 15x15x15 area.

Proposed changes

Make EXP collector's constructor have a range and remove hardcoded capacity and consumption

Approved suggestion

Discord approved suggestion number #514

What is left to do

I need someone to tell me the recipes and or changes to the consumption and capacity of these new machines, so that I can register them into SlimefunItemSetup

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.
  • 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.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@Intybyte Intybyte requested a review from a team as a code owner September 4, 2024 12:42
@github-actions github-actions bot added the 🎈 Feature This Pull Request adds a new feature. label Sep 4, 2024
Copy link
Contributor

github-actions bot commented Sep 4, 2024

Your Pull Request was automatically labelled as: "🎈 Feature"
Thank you for contributing to this project! ❤️

Copy link
Contributor

github-actions bot commented Sep 4, 2024

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: f731ad2

https://preview-builds.walshy.dev/download/Slimefun/4231/f731ad24

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!

@Boomer-1
Copy link

Boomer-1 commented Sep 4, 2024

is there any real application to this? other than wolves killing mobs, and zombified piglins, both farms of which are in tight narrow kill areas, what would be the purpose to go out 15 blocks?

@Sefiraat
Copy link
Member

Sefiraat commented Sep 4, 2024

It's worth noting a few things:

  • You've not actually made SlimefunItems for the two additional SlimefunItemStacks you've made
  • Increasing range when checking for entities can harm performance so the reason would need to be considered vs the new, additional, cost
  • The power requirement to range increase seems low to me, though I could be wrong here

Overall I like the idea of having range within the object so at least addons can extend the class to do something similar here? Would like to hear further opinions before I do anything more.

@Intybyte
Copy link
Contributor Author

Intybyte commented Sep 4, 2024

It's worth noting a few things:

* You've not actually made SlimefunItems for the two additional SlimefunItemStacks you've made

* Increasing range when checking for entities can harm performance so the reason would need to be considered vs the new, additional, cost

* The power requirement to range increase seems low to me, though I could be wrong here

Overall I like the idea of having range within the object so at least addons can extend the class to do something similar here? Would like to hear further opinions before I do anything more.

The addon one was one of the main reason for me too, all the points you made are valid, reason why in my PR description towards the end I said that I am waiting for an adjustement for energy and a recipe, i don't want to register something with null recipe, if the idea is accepted and the recipe made and energy cost made I will of course add them

@JustAHuman-xD
Copy link
Contributor

JustAHuman-xD commented Sep 4, 2024

Yeah the 15x15x15 seems incredibly unnecessary and taxing in performance to boot imo

@Sefiraat
Copy link
Member

Sefiraat commented Sep 4, 2024

It's worth noting a few things:

* You've not actually made SlimefunItems for the two additional SlimefunItemStacks you've made

* Increasing range when checking for entities can harm performance so the reason would need to be considered vs the new, additional, cost

* The power requirement to range increase seems low to me, though I could be wrong here

Overall I like the idea of having range within the object so at least addons can extend the class to do something similar here? Would like to hear further opinions before I do anything more.

The addon one was one of the main reason for me too, all the points you made are valid, reason why in my PR description towards the end I said that I am waiting for an adjustement for energy and a recipe, i don't want to register something with null recipe, if the idea is accepted and the recipe made and energy cost made I will of course add them

So you did! Sorry for that :).

@Intybyte
Copy link
Contributor Author

Intybyte commented Sep 4, 2024

Yeah the 15x15x15 seems incredibly unnecessary and taxing in performance to boot imo

There are many machines with only tier 2, we can just remove the tier 3 if deemed unnecessary, this way we add a new feature and allow addon makers to make their own xp collector easily if needed, still better than the way everything was hardcoded in that class

@Sefiraat
Copy link
Member

Sefiraat commented Sep 4, 2024

Yeah the 15x15x15 seems incredibly unnecessary and taxing in performance to boot imo

There are many machines with only tier 2, we can just remove the tier 3 if deemed unnecessary, this way we add a new feature and allow addon makers to make their own xp collector easily if needed, still better than the way everything was hardcoded in that class

I’d go so far as to say perhaps to leave the new tiers out completely and just have the api opened up like you have done. That’s would be a quick accept as far as I’m concerned.

@Intybyte Intybyte changed the title Feature/more exp collectors api/Open up range field Oct 26, 2024
@Intybyte
Copy link
Contributor Author

Updated now it only opens up the api

JustAHuman-xD
JustAHuman-xD previously approved these changes Oct 26, 2024
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM

…n/items/electric/machines/entities/ExpCollector.java
@WalshyDev WalshyDev merged commit d12ae85 into Slimefun:master Nov 9, 2024
15 checks passed
@Intybyte Intybyte deleted the feature/more_exp_collectors branch November 9, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎈 Feature This Pull Request adds a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants