Implement Multimeter Functionality on Generators #3300
Conversation
| if (operation != null) { | ||
| if (!operation.isFinished()) { | ||
| if (isChargeable()) { | ||
| int charge = getCharge(l, data); | ||
| if (getCapacity() - charge >= getEnergyProduction()) { | ||
| return getEnergyProduction(); | ||
| } | ||
| return 0; | ||
| } else { | ||
| return getEnergyProduction(); | ||
| } | ||
| } else { | ||
| return 0; | ||
| } | ||
| } else { | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
This method is overly long. Can we clean this up a bit?
| } | ||
| } | ||
|
|
||
| private int peekGenerateEnergy(@Nonnull Location l, @Nonnull Config data, @Nonnull BlockMenu inv, @Nullable BlockMenu accessPort, FuelOperation operation) { |
There was a problem hiding this comment.
Can we extract some of the dupe code here? There's no reason to do a bunch of this in both peekGenerateEnergy and generateEnergy
| @@ -96,6 +98,12 @@ public int getGeneratedOutput(Location l, Config data) { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public int peekGeneratedOutput(@Nonnull Location l, @Nonnull Config data) { | |||
| // Solar panels will not change when generating energy | |||
There was a problem hiding this comment.
| // Solar panels will not change when generating energy | |
| // Solar panels do not consume fuel, we can fallback to the original method |
| Slimefun.getLocalization().sendMessages(p, "messages.multimeter-generating", false, str -> str.replace("%stored%", stored).replace("%capacity%", capacity).replace("%generating%", generating)); | ||
| } else { | ||
| Slimefun.getLocalization().sendMessage(p, "messages.multimeter", false, str -> str.replace("%stored%", stored).replace("%capacity%", capacity)); |
There was a problem hiding this comment.
I'd prefer moving multimeter out of messages now that it has multiple messages.
|
|
||
| import javax.annotation.ParametersAreNonnullByDefault; | ||
|
|
||
| import io.github.thebusybiscuit.slimefun4.core.attributes.EnergyNetProvider; |
There was a problem hiding this comment.
Import belongs with the others
| * The method return how much this {@link EnergyNetProvider} is expected to provide to the {@link EnergyNet} | ||
| * and is not to be confused with {@link #getGeneratedOutput(Location, Config)}. This method will not | ||
| * consume any fuel or modify the provider in any way. |
There was a problem hiding this comment.
English here can be improved.
| * The method return how much this {@link EnergyNetProvider} is expected to provide to the {@link EnergyNet} | |
| * and is not to be confused with {@link #getGeneratedOutput(Location, Config)}. This method will not | |
| * consume any fuel or modify the provider in any way. | |
| * This method will return how much this {@link EnergyNetProvider} is expected to provide to the {@link EnergyNet}. | |
| * It is not to be confused with {@link #getGeneratedOutput(Location, Config)}. | |
| * This method <b>should not</b> consume any fuel or modify the provider in any way. |
| * The method return how much this {@link EnergyNetProvider} is expected to provide to the {@link EnergyNet} | ||
| * and is not to be confused with {@link #getGeneratedOutput(Location, Config)}. This method will not | ||
| * consume any fuel or modify the provider in any way. | ||
| * @param l |
There was a problem hiding this comment.
new line
| * @param l | |
| * | |
| * @param l |
There was a problem hiding this comment.
I am not a fan of this approach.
Adding this new method will over-complicate the energy generator implementations unnecessarily.
I don't see this feature as a justifiable reason for this over-complication.
I reckon it would be much less intrusive to have Energy networks simply keep a cache of generated energy amounts per generator which is re-populated on each cycle.
The Multimeter could then just read from the current cache.
WalshyDev
added
the
hacktoberfest-accepted
|
Is this still being worked on or can it be considered stale? |
|
Can be considered stale. Havent had the time to work on it. |
Description
Implementation of #2314
This is going to break any addons which have implementations of
EnergyNetProvider.I considered two approaches to implementing this. One was the way TheBusyBiscuit mentioned in #2314. The alternative is to perform this breaking change.
Proposed changes
Add a new method
EnergyNetProvider#peekGeneratedEnergyAdd a new message
messages.multimeter-generatingRelated Issues (if applicable)
Resolves #2314
Checklist
NonnullandNullableannotations to my methods to indicate their behaviour for null values