Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix plant_nursery pattern #2773

Closed
wants to merge 1 commit into from

Conversation

kocio-pl
Copy link
Collaborator

Resolves #2771.

Currently it's just a PNG fix, since we have no solution or workaround for issue #2750 with SVG.

I have also compacted code a bit.

z17
Before
myggkkbn
After
46mgszgb

@kocio-pl kocio-pl requested a review from imagico August 23, 2017 20:34
@kocio-pl kocio-pl mentioned this pull request Aug 23, 2017
Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying to warm up to the whole do-ocracy and quantity-over-quality thing here but this is really beyond my ability at the moment.

Looking at either your preview image or the pattern PNG should immediately tell you that something went seriously wrong here. And this is something you should see before someone else points it out. Otherwise you should really consider slowing down and taking more time evaluating and checking your own changes before you ask others to review them.

@matthijsmelissen
Copy link
Collaborator

@imagico Please try phrasing your comments in a constructive way. We have people from different backgrounds here, and what might be obvious to one person might not be obvious to someone else.

And do you mean something is wrong with the 'after' image? Because I don't see this either.

@matthijsmelissen
Copy link
Collaborator

Actually something seems wrong with the pixel alignment, is that what you mean?

@kocio-pl
Copy link
Collaborator Author

I'm still not sure if this is correct now - it's not that different for me than previous "after" version when looking at 1x scale, BUT when zooming in 3-5x it looks correct now - pixels are not misaligned. Probably this was something with changing "render (px aligned)" to "render (inlined & aligned)":

ywvuw44w

@matthijsmelissen
Copy link
Collaborator

Yes, that looks much better. Thanks for spotting @imagico!

@kocio-pl
Copy link
Collaborator Author

@imagico Please remember that no one is error-proof. The original pattern was suggested by you and the problem has been spotted by Matthijs only few days ago, when I was trying to recreate it in SVG. It's just hard to see one's own mistakes, so it's good if someone else takes a look and helps. IMO at the end of the day it's the output that counts, not who made a mistake and how many of them.

@imagico
Copy link
Collaborator

imagico commented Aug 24, 2017

It is a matter of fairness, you make one PR after the other with apparently next to no own QA and expect others to sort out the errors (or not - but then it is unfair to the downstream style users). If you were a new contributor i would without further comment point out the errors in the first few cases and if this happens repeatedly i would hint that a bit more thoroughness in work would be good (and would point them to #2291 (comment)). You have made hundreds of changes over the last years so i think it can be expected there is some learning curve and an improvement in quality of the changes and in self critical awareness of one's own limitations.

As far as the old pattern is concerned i was well aware of its limitations when i suggested it - this is obvious with a bit of understanding of integer math and the triangular/hexagonal pattern geometry. This is a design decision to be made. I merely pointed out the possibility. To be clear: I make errors and have my limitations as well although these tend to be more on the level of communication (like i am painfully aware of my inability to convincingly communicate my point of view in #2654). On the technical level: One of the primary principles i try to adhere to with my active work here is to deal with and fix issues with my own changes before moving on to the next. I have rarely had more than one open PR so i am able to concentrate on one thing and not try to juggle several subjects at once at the cost of quality.

Anyway - back to the subject: you write:

Resolves #2771.

This is probably not immediately visible at the first look if you are not aware of the mathematical impossibility but that is not a correct statement (unless i completely misunderstood #2771).

@matthijsmelissen
Copy link
Collaborator

@imagico I think you stand aline in this, I have not noticed a structural shortcoming in the quality of kocio-pl's pull requests.

And I think you indeed misunderstand #2771, this issue was pointed out by me in a comment and I can this PR solvee it.

@kocio-pl
Copy link
Collaborator Author

We can play the game of "fairness", but it won't get us anywhere IMO, so I simply refuse to take part in it. I can perfectly understand if you don't like something, but that's not a moral obligation for anybody. The same for my expectations and feelings - I don't like some other things, but that doesn't make others "unfair". Or does it?

We all learn. Because of working with osm-carto I'm now better at many things, particularly in CartoCSS, SQL, git, Inkscape, icon design, cartography, project management and decision making. I'm also better at "pixel peeping", yet this is not where I feel too strong. But I've noticed that you are also learning to communicate better. My expectations are higher (one can say: "it's expected that talking should be easy") - but so what? We just don't live up to each other expectations and I can live with it, I just care more for a better map as I understand it. We can still work together where our visions adhere or we have some good will to help each other where we don't mind - and that's enough for me.

On the technical level: One of the primary principles i try to adhere to with my active work here is to deal with and fix issues with my own changes before moving on to the next. I have rarely had more than one open PR so i am able to concentrate on one thing and not try to juggle several subjects at once at the cost of quality.

OK, that's what you believe is good for you. But there are also serious drawbacks I see and for me they outweigh the perceived quality gains:

  • it's hard to understand what you have constructed over time, simply because nobody was following you and after some time probably nobody else even remembers the whole thing
  • after spending this time designing alone you expect others to accept the whole construction, while some parts can be always removed or replaced
  • "unlimited QA" for one thing means lack of time for fixing the issues that are already here, so in practice it means accepting the fact they are unresolved, so overall quality suffers
  • avoiding mistakes at all cost makes it harder for others to take any risk and learn things by asking, because now it's no more just issue fixing, but also ego-play (because nobody - including me - likes to be blamed)
  • avoiding mistakes and lack of communication can lead to a project halt

That's why I don't want to work your way and prefer mine, but you make your own decisions and we all have some habits. So be it.

BTW: "quantity over quality" as you claim is nice rhetorical figure (an exaggeration), but for me it's balancing both things. "Quality, stupid!" is not an end-of-story argument one can always win any debate. QA is just a part of the project.

@kocio-pl
Copy link
Collaborator Author

kocio-pl commented Sep 10, 2017

This code fixes artifacts in plant_nursery pattern and makes it regular. Are there any other problems with this code or pattern?

@kocio-pl
Copy link
Collaborator Author

I'm not interested in fixing patterns right now if I don't see the clear advantage until we decide what to do with tiles >256 px and SVG format, so I'm dropping this code.

@kocio-pl kocio-pl closed this Sep 30, 2017
@kocio-pl kocio-pl deleted the plant_nursery-fix branch September 30, 2017 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Artifacts in plant_nursery pattern
3 participants