View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000030Industrial-Craft²E-Net, cabling, storage/transformer blockspublic2012-11-03 06:532012-11-17 15:08
Reportermatjam Assigned ToRichardG  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Platformx86OSWindowsOS Version7
Summary0000030: EU Storage Devices do not emit redstone reliably
DescriptionAn MFSU or similar device will not update redstone near it when it's state changes. You can force an update by destroying the redstone and replacing it; doing so causes the MFSU to either send a signal or not depending on it's mode.
Steps To Reproduce1. Start a clean creative world.
2. Spawn an MFSU.
3. Set to "Emit if Empty"
4. Place Redstone dust next to MFSU
5. Note that redstone will be correctly set on.
6. Set MFSU to "Emit if Full"
7. Note that redstone will INCORRECTLY still be on.
8. Remove the redstone dust.
9. Replace the redstone dust. (redstone will be off)
10. Fully charge the MFSU.
11. Note that redstone will INCORRECTLY still be off.
TagsNo tags attached.
Minecraft Version

Relationships

has duplicate 0000078 resolvedRichardG Redstone Behaviour Button on Storage Does Not Work 
has duplicate 0000084 resolvedRichardG Energy Storage not updating adjacent redstone wire 

Activities

RawCode

2012-11-03 08:45

reporter   ~0000064

it shoud notify all blocks around, not only itself inside update TileEntity code.

matjam

2012-11-06 11:51

reporter   ~0000101

In TileEntityElectricBlock.java:187 (deobf source)

        if (var2 != this.isEmittingRedstone())
        {
            this.worldObj.notifyBlocksOfNeighborChange(this.xCoord, this.yCoord, this.zCoord, this.worldObj.getBlockId(this.xCoord, this.yCoord, this.zCoord));
        }

remove the if clause; it will never evaluate true.

Just have:

            this.worldObj.notifyBlocksOfNeighborChange(this.xCoord, this.yCoord, this.zCoord, this.worldObj.getBlockId(this.xCoord, this.yCoord, this.zCoord));

And this fixes the issue. :)

FunnyMan3595

2012-11-06 21:41

reporter   ~0000117

matjam, your logic is faulty. That clause has an important purpose; it reduces the overhead of every placed electric block.

What's going on here is that we're monitoring a value to see if it changes. That means that we want to store the existing value, do things that might change it, and then check the value to see if it's matched what we stored.

We store it like so:
(A) boolean wasEmittingRedstone = this.isEmittingRedstone()

And then check it like this:
(B) if(wasEmittingRedstone != this.isEmittingRedstone()) {^

Unfortunately, about 5 lines later, we have this:
(C) this.energy -= this.output - EnergyNet.getForWorld(this.worldObj).emitEnergyFrom(this, this.output);


If you go check isEmittingRedstone(), you'll see that it depends on the value of this.energy. So we've modified the value *after* checking it, but *before* we store it again. Follow the logic through a run where we were full, but then output some energy. And let the condition be "output redstone when full".

1. Start of first tick.
2. (A) stores true, because we were full.
3. (B) compares true against true, because we're still full. No need to notify anyone.
4. (C) reduces the stored energy. We're no longer full.
5. Start of second tick.
6. (A) stores false, because we're not full.
7. (B) compares false against false, because we're still not full. No need to notify anyone. (Or so the code says.)

Because (C) happened outside of (A)-(B), we didn't detect the change, so we never notified adjacent blocks of it. Removing the check in (B) entirely will cause us to send the update in tick 2, when it has changed, but also in tick 1 and *every other tick*. So that batbox you left somewhere-or-other that's had exactly 2,408 EU for the last three days is still causing pointless overhead every tick by notifying all the adjacent blocks that it "changed".

matjam

2012-11-07 00:00

reporter   ~0000120

At the top of the method, it has

 boolean var2 = this.isEmittingRedstone())

it then does

 if (var2 != this.isEmittingRedstone())

that's never going to evaluate true. It will never send an update.

So, work needs to be done to only send updates when needed. I understand the intent, there. It's probably just a typo. But that's the bit that's borken.

FunnyMan3595

2012-11-07 00:05

reporter   ~0000121

Yes, you said that before. You're still wrong, and I've already said why. The problem is not the check, it's that one of the things that should be inside it... isn't.

matjam

2012-11-07 00:15

reporter   ~0000122

Last edited: 2012-11-07 00:16

 I don't have the familiarity with the code base. It's what I worked out with 5 minutes looking at the deobfuscated code. So, I'm okay with being wrong. :P

matjam

2012-11-07 00:36

reporter   ~0000123

Oh, and btw, thanks for taking a look at this bug! If you do have access to the unobfuscated source, it'd be good to see some kind of fix for this in the next release. Hard to automatically shut off a reactor when my MFSU is full; annoying to have to manually turn it on and off ;)

matjam

2012-11-07 04:57

reporter   ~0000124

Last edited: 2012-11-07 05:02

 Okay I reworked my suggested fix; it now behaves like this (debug statements obviously, not there in this fix, just to illustrate behaviour.)

2012-11-07 15:48:38 [INFO] [STDOUT] wasEmittingRedstone : false isEmittingRedstone(): false
2012-11-07 15:48:38 [INFO] [STDOUT] wasEmittingRedstone : false isEmittingRedstone(): true
2012-11-07 15:48:38 [INFO] [STDOUT] wasEmittingRedstone : true isEmittingRedstone(): true


1) New field in the class definition for TileEntityElectricBlock:

    public boolean wasEmittingRedstone = false;

2) Remove "wasEmittingRedstone" (or in my case, var2) from TileEntityElectricBlock updateEntity().

3) Remove the previous if (wasEmittingRedstone != this.isEmittingRedstone) { ... } section.

4) Add the following to the end of TileEntityElectricBlock updateEntity().

        if (wasEmittingRedstone != this.isEmittingRedstone())
            this.worldObj.notifyBlocksOfNeighborChange(this.xCoord, this.yCoord, this.zCoord, this.worldObj.getBlockId(this.xCoord, this.yCoord, this.zCoord));
        
        wasEmittingRedstone = this.isEmittingRedstone();

Should meet your requirements? Have tested, seems to work correctly. Appreciate feedback :)

FunnyMan3595

2012-11-07 05:13

reporter   ~0000125

Much better, yes. Without testing it myself, I can't swear to it working, but in theory that's a perfect fix to the problem I isolated.

For the record, I'm not that much better off than you, I just have a better decompilation and enough mental context to make the jump to understanding the coder's intent, not just the code itself.

I'm also not convinced your initial assertion was correct. There's so much intervening code that I'm not sure if isEmittingRedstone() can change between when it's cached and when it's checked later. But you definitely did have the right idea in one way: the check you identified wasn't always tripping when it should. And a solution like your current one makes things so much easier, because there's obviously no gap where a change could happen and not be detected.

In short: Yeah, that looks about right. Might need attention when its chunk is loaded, though.

matjam

2012-11-07 06:24

reporter   ~0000126

I think it's because injectEnergy() is called by ENet stuff. Doesn't appear to be any other method that can modify the stored energy in the object.

RawCode

2012-11-07 06:58

reporter   ~0000127

just check how vanilla redstone stuff works, it update EVERY ajucent block, not itself, there is no reason to make this different

matjam

2012-11-07 07:56

reporter   ~0000136

notifyBlocksOfNeighborChange does that:

    /**
     * Notifies neighboring blocks that this specified block changed Args: x, y, z, blockID
     */
    public void notifyBlocksOfNeighborChange(int par1, int par2, int par3, int par4)
    {
        this.notifyBlockOfNeighborChange(par1 - 1, par2, par3, par4);
        this.notifyBlockOfNeighborChange(par1 + 1, par2, par3, par4);
        this.notifyBlockOfNeighborChange(par1, par2 - 1, par3, par4);
        this.notifyBlockOfNeighborChange(par1, par2 + 1, par3, par4);
        this.notifyBlockOfNeighborChange(par1, par2, par3 - 1, par4);
        this.notifyBlockOfNeighborChange(par1, par2, par3 + 1, par4);
    }

FunnyMan3595

2012-11-07 14:05

reporter   ~0000145

matjam: Good point. injectEnergy is likely called outside of the entire method, so you're definitely on the right track for solving the bug.

RawCode: You're barking up the wrong tree. matjam spotted the code responsible, and I decoded its intent for him. The bug is basically as solved as it can be without one of the IC2 devs turning up to implement the fix.

RawCode

2012-11-09 04:29

reporter   ~0000163

doing same as vanilla always works well

RichardG

2012-11-17 15:08

administrator   ~0000230

Fixed in the 1.4.4 version.

Issue History

Date Modified Username Field Change
2012-11-03 06:53 matjam New Issue
2012-11-03 06:53 matjam Status new => assigned
2012-11-03 06:53 matjam Assigned To => Player
2012-11-03 08:45 RawCode Note Added: 0000064
2012-11-03 13:04 RichardG Assigned To Player =>
2012-11-06 11:51 matjam Note Added: 0000101
2012-11-06 21:41 FunnyMan3595 Note Added: 0000117
2012-11-07 00:00 matjam Note Added: 0000120
2012-11-07 00:05 FunnyMan3595 Note Added: 0000121
2012-11-07 00:15 matjam Note Added: 0000122
2012-11-07 00:16 matjam Note Edited: 0000122
2012-11-07 00:36 matjam Note Added: 0000123
2012-11-07 04:57 matjam Note Added: 0000124
2012-11-07 05:02 matjam Note Edited: 0000124
2012-11-07 05:13 FunnyMan3595 Note Added: 0000125
2012-11-07 06:24 matjam Note Added: 0000126
2012-11-07 06:58 RawCode Note Added: 0000127
2012-11-07 07:56 matjam Note Added: 0000136
2012-11-07 14:05 FunnyMan3595 Note Added: 0000145
2012-11-09 04:29 RawCode Note Added: 0000163
2012-11-17 15:08 RichardG Note Added: 0000230
2012-11-17 15:08 RichardG Status assigned => resolved
2012-11-17 15:08 RichardG Resolution open => fixed
2012-11-17 15:08 RichardG Assigned To => RichardG
2012-11-17 15:09 RichardG Relationship added has duplicate 0000078
2012-11-20 20:59 RichardG Relationship added has duplicate 0000084