![[de]](/mwf/flags/de.png "Germany")
Short notice: I am currently still refactoring and fixing the pump, so please do not touch it.
Zapper, could you check out the pumpfix branch and test the pump? I am done with the refactoring and the pump works so far except for the power usage. The pump only gets the callback OnEnoughPower and never OnNotEnoughPower even though it consumes power all the time.the version currently in the branch still has debug logs and messages on, so it should be easy for you to see what the pump currently does or tries to do.
>and never OnNotEnoughPower even though it consumes power all the time.
Maybe you have enough power all the time? *shrug*
I'll have a look at it, sure
Well, now that is weird! But good to know, I hope I can reproduce that
ERROR: '->': no function "CanInsertMaterial" in object "Object(81)".
by: Object(81)->HasLiquidToPump() (Objects.ocd\Structures.ocd\Pump.ocd\Script.c:356)
by: Object(81)->CheckState() (Objects.ocd\Structures.ocd\Pump.ocd\Script.c:255)@.@ *fix*
Fixed now (except for the CanInsertMaterial/InsertMaterial thing). I did not remove the debug-messages etc. yet. You could quickly test it again to see whether I missed anything and then merge it back to master.
Hmm, your fix looks more like a workaround of a bug in the power system than a fix. What you did is delay the state checks and set the power producer/consumer states conditionally only when they really changed. Shouldn't the Make/Unmake Power Producer/Consumer be robust enough that you can set the states even if everything stays the same?Or am I missing something, what was the problem?
>What you did is delay the state checks and set the power producer/consumer states conditionally only when they really changed
MakePowerConsumer should not be called from The On*EnoughPower methods. I thought I documented that somewhere, but I might have not done that.
>Shouldn't the Make/Unmake Power Producer/Consumer be robust enough that you can set the states even if everything stays the same?
You had some general logical problems in that code. For example, OnEnoughPower is obviously never called when you are a power producer and not a consumer, therefore your
powered was never set to true in that case - this is the main reason for the biggest part of the patch.UnmakePowerConsumer/Producer are robust enough, but when you always remove- and add the object as a power consumer, the light-bulb effect is going to be played again, for example. And since removing and re-adding is not even what you should want in that case, I believe just changing the requested power when and as necessary is the right way.
For example, the checks [if (power_used > 0) UnmakePOwerConsumer();] in line 406ff could probably be skipped without changing anything, but I think that the checks help you read and understand the code.
> MakePowerConsumer should not be called from The On*EnoughPower methods. I thought I documented that somewhere, but I might have not done that.
Could the solution here be to make the On*EnoughPower callbacks each at the end of the function? So after the system updated to the new power consumer state?
> but when you always remove- and add the object as a power consumer, the light-bulb effect is going to be played again, for example
But this could be fixed in the power system right? If the power consumption for example is already 10 and didn't change in this call to MakePowerConsumer, then do not redisplay the bulbs.
>Could the solution here be to make the On*EnoughPower callbacks each at the end of the function? So after the system updated to the new power consumer state?
Possible, but I don't see why you would want to change the state in that callbacks anyway :x
>But this could be fixed in the power system right? If the power consumption for example is already 10 and didn't change in this call to MakePowerConsumer, then do not redisplay the bulbs.
Yes, the power system treats it exactly like you say. But the power system is powerless when you call UnmakePowerConsumer before setting the new power ;)
>Possible, but I don't see why you would want to change the state in that callbacks anyway :x
Because when it does not have any power, it should stop pumping.
I may have read that wrong, but as I understood it, you always updated the power usage (with UnmakePowerConsumer & MakePowerConsumer) even if nothing changed.That means, you stopped requesting power (
UnmakePowerConsumer) in OnNotEnoughPower through CheckState and UpdatePower (or however the functions are called).PS:
When I said "[...] I don't see why you would want to change the state in that callbacks [...]", I meant the state of being a power consumer, not the internal state of the pump. That came across wrong, I suppose
> I may have read that wrong, but as I understood it, you always updated the power usage (with UnmakePowerConsumer & MakePowerConsumer) even if nothing changed.
For example when power consumption changes from 18 to 20, I called
UnmakePowerProducer(); and MakePowerConsumer(20);. UnmakePowerConsumer was not called.
Found the problem. You where right about the water pixels sliding out of the map. I am on it. @ CanInsertMaterial
What exactly was broken?Btw: You removed the InsertMaterial query parameters? How can you then determine how far InsertMaterial will push the material up because the target is drowned in liquid? (otherwise, you can gain infinite energy by pumping in circles).
GetPumpHeight() ist still wrong. It does not slide sideways for the material check like InsertMaterial does. Thus, you can still gain infinite energy by pumping in circles.
How would the sideway slide effect the height and thus the power produced?
The water inserted by InsertMaterial slides diagonally upwards, so it will find a top end of the water body further up. But the pump will calculate power needed as if the water were inserted straight up only.Edit: I'm referring to this: http://git.openclonk.org/openclonk.git/blob/refs/heads/pumpfix:/src/landscape/C4Landscape.cpp#l833
I don't really understand what these two lines do. I understand it like this: Slide straight up, but in the case that any neighboring pixel is sky/tunnel, slide one px in that direction and stop.
I thought it's supposed to go up diagonal caves. But looking at the code, I guess it would have to check "<=mdens" for that. So my guess is that the "primitive slide (1)" has been there since Clonk Planet and has never actually worked.In any case, the pump height procedure should use the same code path as the insertion procedure to determine it's position. There's also the code in FindMatPathPush which might search for an entirely different insertion position.
Maybe make that InsertMaterial() (or whatever it was called) function return the position, where the inserted pixel finally ends up.
Well, that's what I did. But it was removed in the pumpfix branch and not readded to the CanInsertMaterial function.I think both functions should have that output parameter. It's useful for CanInsertMaterial because you need it for the pump height. But it's also useful for the InsertMaterial function in case you want to do extra stuff at the insertion position (e.g. add sprinkler effects).
> So my guess is that the "primitive slide (1)" has been there since Clonk Planet and has never actually worked.
In 2001 those lines looked like this:
if (GBackDensity(tx-1,ty)<mdens) tx--;
if (GBackDensity(tx+1,ty)<mdens) tx++;
So same without the pointer fu. Does anyone have a concern against throwing that part out as I don't see the sense of that part?
The pointers are for the output parameters to determine where the liquid is actually inserted. If you don't like it, you can replace it with local variables and write them back into the pointers on function exit. But I believe there are multiple exit points, so this might be a bit tedious.
Powered by mwForum 2.29.7 © 1999-2015 Markus Wichitill
