Level Emitter not working correctly #229

Closed
FireBall1725 opened this Issue Oct 3, 2014 · 10 comments

Projects

None yet

6 participants

@FireBall1725
Member

When 2 level emitters on the same network are configured to use energy mode, and have the same value specified only the first one to register will work, while the other one will do nothing. If you change the numbers so they are off by 1 then they both will work

@FireBall1725 FireBall1725 self-assigned this Oct 3, 2014
@FireBall1725 FireBall1725 added this to the rv2 milestone Oct 3, 2014
@FireBall1725
Member

PartLevelEmitter.java

if ( myEnergyWatcher != null )
myEnergyWatcher.add( (double) reportingValue );

seems to be where it registering the reporting value, need to look into that more...

@thepaperpilot
Contributor

I've been tackling this problem. I found the issue was it trying to add the different EnergyThresholds to a TreeSet, which rejected them because it doesn't want duplicates (and it only goes off the energy value, as it should. That isn't the part we can change). I can confirm that is absolutely the issue, I actually had initially thought the line master801 referenced was the issue as well, but determined that has nothing to do with it.

I solved this by making it a TreeMultiset, which allows for the duplicates.

But, this doesn't work completely. I'm not sure if its a problem created by the above solution or a problem that has always applied but was undetectable because of the root problem, but here goes:
the TreeMultiset is being created over and over, and each "interest" is added to one of these individual instances, and then another instance of the first interest is added to the first TreeMultiset instance (which is being called from somewhere?), resulting in one instance or TreeMultiset with a bunch of copies of one interest, and a bunch of unreferenced TreeMultisets with each of the other interests.

hopefully that explains the code well enough. I'm taking a break from this bug because its driving me nuts- if anyone else would like to attempt this, I've made a branch here that has the TreeMultiset instead of TreeSet.

@master801

@thepaperpilot
Nice! You fixed it! :D By the way, the only reason why I referenced that line, was because fireball1725 didn't link it when he posted this report.

@thepaperpilot
Contributor

well, "fixed it".

On Sun, Oct 5, 2014 at 9:06 PM, master801 notifications@github.com wrote:

@thepaperpilot https://github.com/thepaperpilot
Nice! You fixed it! :D By the way, the only reason why I referenced that
line, was because fireball1725 didn't link it when he posted this report.


Reply to this email directly or view it on GitHub
#229 (comment)
.

-Anthony Lawn

@master801

@thepaperpilot

Close enough. XD

@yueh
Member
yueh commented Oct 6, 2014

I am still considering doing a refactoring pass on the level emitter.

My current considerations are, not final and maybe not possible:

  • Level Emitters should not hold a reference to every watcher (type).
    • Replace it with a common interface.
  • Level Emitters are only acting on a threshold (change).
    • Move the heavy lifting to the watcher itself.
    • (Threshold?)WatcherHost would only need method call on a condition change.
  • Only one watcher per interest value, but multiple WatcherHost (Observers) per watcher
    • There is no real need to have multiple objects watching the exactly same thing.
    • Just pass the change to any observer for the specific value.
    • Still questionable how to handle Upgrades like the fuzzy card, if the handling gets pushed into the watcher itself and can notify multiple Level Emitter with and without Upgrades (or observers in general).
    • This might require a Factory to get a cleaner approach to handle registering the observers and also destroying superfluous watchers. But it might be possible to expose it through the API. So other mods could reuse it and do not need to implement their own watchers/observers.
  • I need to look into things like the Storage Monitor
    • The underlying behaviour might be very similar and watchers are identical to the monitors. Only indicating passing a thresheld on a value instead of showing the value.
    • Maybe this could also enable monitors displaying the stored energy, requesting crafting for the displayed item (dropping after finished in case of conversion monitor? Fuzzy Monitors?)

Pretty sure I missed some things while quickly scanning of the the code.
Like the might be already existing some implementation which are similar to it.

@thepaperpilot
Contributor

I should add I tried ArrayList before trying the TreeMultiSet, and made a custom "find values in range" function for the subList function. This also fixed the issue with only having 1 interest, but also had the issue with the interests all being duplicates. Therefore, it probably wasn't an issue created by using TreeMultiset, and was rather an underlying issue that was exposed.

I agree with @yueh, those proposed changes sound much faster/simpler. Although, I would handle the fuzzy card with a separate instance of the watcher, and call both. That just seems like the easiest way, and isn't affecting the overhead that much.

@thatsIch thatsIch modified the milestone: rv3, rv2 May 7, 2015
@TheJulianJES
Contributor

Is there any progress on this issue? :D

@thatsIch
Member
@yueh yueh closed this in #2693 Dec 14, 2016
@yueh yueh added a commit that referenced this issue Dec 14, 2016
@yueh yueh Replaced Watcher using Collection with a more fitting interface (#2693)
Replaced the watchers for energy, storage and crafting with a more fitting interface compared to a common collection.

Fixes #229
fb79fd2
@phit phit added a commit to Stonebound/Applied-Energistics-2 that referenced this issue Dec 19, 2016
@yueh @phit yueh + phit Replaced Watcher using Collection with a more fitting interface (#2693)
Replaced the watchers for energy, storage and crafting with a more fitting interface compared to a common collection.

Fixes #229
41d44f7
@phit phit added a commit to Stonebound/Applied-Energistics-2 that referenced this issue Dec 19, 2016
@yueh @phit yueh + phit Replaced Watcher using Collection with a more fitting interface (#2693)
Replaced the watchers for energy, storage and crafting with a more fitting interface compared to a common collection.

Fixes #229
e3fecbc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment