Closed MM1nd closed 6 years ago
To further illustrate the point, here is the output as it is now:
Note how the upper and left side of the small islands are darker.
This is what I think would be correct:
Notice how small islands are anti-aliased symetrically. One might argue that real islands aren't symmetrical. However they are also not predictably asymmetrically towards the north west. Anti-aliasing cannot magically conjure information from nothing.
I think we can change the blessed images when it makes sense: the goal of having images is that we can manually check them and decide what it makes sense. I would not be stopped by a minor change in the final output when it is well thought and reasoned as in this case.
On 8 October 2017 at 17:46, MM1nd notifications@github.com wrote:
To further illustrate the point, here is the output as it is now: [image: am_incorrect] https://user-images.githubusercontent.com/1690444/31318282-e379a284-ac4f-11e7-8f65-cf185bc51d05.png Note how the upper and left side of the small islands are darker.
This is what I think would be correct: [image: am_correct] https://user-images.githubusercontent.com/1690444/31318283-e53c67e6-ac4f-11e7-97c4-4b45b900eaa2.png
Notice how small islands are anti aliased symetrically. One might argue that real islands aren't symmetrical. However they are also not predictably asymmetrically towards the north west. Anti-liasing cannot magically conjure information from nothing.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Mindwerks/worldengine/issues/250#issuecomment-335015509, or mute the thread https://github.com/notifications/unsubscribe-auth/AAazJlV7fo1t9cpUvVNWJs268TY5chbwks5sqO5xgaJpZM4PuILT .
-- Website at https://tomassetti.me GitHub https://github.com/ftomassetti
In that case I'll make a PR of what I have.
It seems I can only close this "red". Can you mark it as resolved?
Issues can just be closes, while PR can be merged or closed, so closing it is the right thing to do here
Hi
I think we might have a problem, which took me way too long to hunt down, so let me back up a bit.
This is
common.anti_alias
as it was before I turned it into a convolution:Notice a few things:
map[y,x]*2
part that does not depend on the previous step but keeps the result somewhat close to the original. Unfortunately theoriginal
parameter is very confusingly named in that respect.py
andpx
are mapped by the modulo operator to the half open interval from 0 toheight
orwidth
respectively.range(0, width)
in python notation. This makes for what we might call circular boundary conditions (i. e. this particular operation treats the map as if it were a torus)I'm no expert in information theory, but all of this makes perfect sense, both sematically and mathematically. I think we can safely assume that this implementaion is correct. The current implementation shows exactly the same behaviour, only faster.
Compare that with
draw_ancientmap.anti_alias
as it is now:Again notice a few points:
common.py
that I find it very likely that it was copied and then edited.common.anti_alias
Unfortunately it doesn't do the same thing at all. The points in the first list all fail here:
target[y, x][0] * 2
refers to the result from the previous step, not the original state before any anti-aliasing. This might be intentional, but I kindof assume it's not, because there is no point in having seperate functions for the whole operation and individual steps, if not to take advantage of python scoping rules. Ironically this is far less of a problem, than one should assume, because the whole thing is only ever called withstep=1
still I argue it would show incorrect behaviour it ever called with larger values forstep
.py
andpx
are clipped by the if to the open intervall from 0 toheight
andwidth
respectively.range(1,height)
in python notation. Here I cannot even imagine circumstances where that would be intentional, I think it is simply a mistake (easily explainend because only very rarely the>=
operator is what one wants). Again note that ironically this does not show because this close to the boundary our maps are water and there is nothing to anti-alias there, Still, incorrect code is incorrect.common.anti_alias
is correct, this is not, regardless the intentions.Now for the hard part:
Long story short, I think this is not a correct implementation of the anti-alias operation and the aforestated points should be fixed. This would mean changing the "blessed" images so that the tests still pass. The test, IMO, is currently testing the wrong thing. Which is very unfortunate.
Full disclosure: Since the result is currently dependent on iteration order, it cannot be made fast. That's just a simple fact of life. So I see some risk that this comes across as "Alex wants to change the blessed image so that his fancy convolution works". I want to be very clear that that is not where I'm coming from. That is why I presented this in comparison with the naive but correct implementation, not the convolution. I'm genuinely convinced that this version is incorrect as it is implemented now.
Ideally, I think one of you should implement this correctly, i. e. after the pattern of the original
common.anti_alias
, and generate a new blessed image so that any risk of me tailoring the test to accept what I want to implement anyways, is mitigated. Should be an hour of work, tops. Maybe I'm too careful here.Cheers
Alex