Opened 13 years ago

Last modified 13 years ago

#3430 assigned patch

fill pattern missing for point markers in new symbology engine

Reported by: nirvn Owned by: sunilkcube
Priority: major: does not work as expected Milestone: Version 1.7.0
Component: Symbology Version: Trunk
Keywords: Cc:
Must Fix for Release: No Platform: All
Platform Version: Awaiting user input: no

Description

It`s not possible with the new symbology engine to set the fill pattern (i.e. vertical line, horizontal line, etc.) of a point marker.

The feature is available in the old symbology engine and can be quite useful at times (for e.g. to create a circle surrounding a marker with a specific radius length using map unit).

Will it be reimplemented?

Cheers

Attachments (8)

qgis-point-symbols.jpg (124.8 KB ) - added by nirvn 13 years ago.
(arrow points at lost features)
qgis-point-new_symbols.jpg (39.6 KB ) - added by nirvn 13 years ago.
qgis-example-villages+perimeter.jpg (110.3 KB ) - added by nirvn 13 years ago.
patch_for_bug_#3430.diff (11.6 KB ) - added by sunilkcube 13 years ago.
new_patch_for_bug_#3430.diff (11.4 KB ) - added by sunilkcube 13 years ago.
New patch
fills.PNG (11.2 KB ) - added by Alister 13 years ago.
patch_for_bug_#3430_alister.diff (13.8 KB ) - added by Alister 13 years ago.
patch_for_bug_#3430_alister 25-5-11.diff (15.1 KB ) - added by Alister 13 years ago.

Download all attachments as: .zip

Change History (32)

by nirvn, 13 years ago

Attachment: qgis-point-symbols.jpg added

(arrow points at lost features)

by nirvn, 13 years ago

Attachment: qgis-point-new_symbols.jpg added

comment:1 by nirvn, 13 years ago

Since the new symbology engine will be the default in 1.7, I think devs should look at this lost of feature from old to new symbology. I've attached screenshots (old engine, new engine, and one map example) in case my description above was unclear.

comment:2 by nirvn, 13 years ago

Platform: DebianAll
Version: Trunk

comment:3 by sunilkcube, 13 years ago

Owner: changed from nobody to sunilkcube
Status: newassigned

by sunilkcube, 13 years ago

Attachment: patch_for_bug_#3430.diff added

comment:4 by sunilkcube, 13 years ago

The patch has been added above. Can anyone please kindly review the patch and if you have any comments let me know , I will soon work on your comments and try to resolve that ?. Thanks .

comment:5 by lutra, 13 years ago

Type: bugpatch

comment:6 by Alister, 13 years ago

Thanks Sunil; I'm not someone who can review the patch, but it seems to work well :)

comment:7 by Alister, 13 years ago

I find that I can switch a layer from old symbology to new symbology successfully, but not back again. The patch could be improved slightly by allowing it to switch back, but I think this is a very minor issue (probably not worth the effort to fix) if old symbology is to be removed from QGIS soon.

e.g.

  • select the first point symbol (circle) in old symbology, set the fill options to "None", Apply
  • switch to new symbology and Apply - the markers should stay the same (apart from outline width, which is #3458)
  • switch to old symbology - the markers will now have a solid fill

comment:8 by Alister, 13 years ago

Also, I'm pretty sure this part of your patch breaks the dialogue somewhat:

-  connect( spinAngle, SIGNAL( valueChanged( double ) ), this, SLOT( setAngle() ) );

comment:9 by Alister, 13 years ago

I'm guessing you actually meant to delete the line above that, as it is the same as the line the patch adds below, so the same line will appear twice if the patch is used.

in reply to:  9 comment:10 by sunilkcube, 13 years ago

Replying to Alister:

I'm guessing you actually meant to delete the line above that, as it is the same as the line the patch adds below, so the same line will appear twice if the patch is used.

Thanks for reviewing the patch. Yes you are right I have done mistake while patching. I have attached a new patch can you please kindly review this one also ?

by sunilkcube, 13 years ago

New patch

comment:11 by Alister, 13 years ago

Hmmm. I didn't realise transparency wasn't working. I'm guessing uncommenting this in the new patch was supposed to fix it:

// selColor.setAlphaF( context.alpha() );

But transparency for the symbol fill still doesn't work for me... unless QGIS hasn't rebuilt properly.

comment:12 by Alister, 13 years ago

Like I say, I can't "review" the patch as I have almost no knowledge of C++, QT or the internals of QGIS.

I've noticed another thing that could be improved though: when you select a feature, the fill style is not applied to it.

comment:13 by Alister, 13 years ago

see attached screenshot:

  • fill style is not applied to selected features (actually, it looks like selected points are rendered by simply filling each symbol layer with yellow, so if transparency is applied the yellow is darker where two symbol layers overlap). I don't think this is a significant problem.
  • transparency is only applied to borders, not fills.

by Alister, 13 years ago

Attachment: fills.PNG added

comment:14 by Alister, 13 years ago

  • transparency is only applied to borders, not fills.

But it is applied to the yellow fill for selected features, as you can see :)

comment:15 by Alister, 13 years ago

  • fill style is not applied to selected features (actually, it looks like selected points are rendered by simply filling each symbol layer with yellow, so if transparency is applied the yellow is darker where two symbol layers overlap). I don't think this is a significant problem.
  • transparency is only applied to borders, not fills.

OK, that was easier than I expected. I've had a look at it, and fixed those things, and also what I'm pretty sure was a pre-existing mistake in this line: QColor selPenColor = selBrushColor == mColor ? selBrushColor : mBorderColor; I had to tidy/rearrange the code there so I could understand it :)

I added a number of comments, mostly about inconsistencies compared with how QGIS renders polygon layers. Can someone else look at these and tell me what they think?

There is one reasonably annoying bug: {{{"no fill" doesn't work if brushColor = penColor e.g. if the symbol is a circle, the whole bounding box (is that what it's called?) of the circle is filled.}}} I'm guessing to fix this an if statement is required something like the one I commented out because that method didn't work. But I don't know what to put in it!

by Alister, 13 years ago

comment:16 by Alister, 13 years ago

Let's try that post again!

  • fill style is not applied to selected features (actually, it looks like selected points are rendered by simply filling each symbol layer with yellow, so if transparency is applied the yellow is darker where two symbol layers overlap). I don't think this is a significant problem. - transparency is only applied to borders, not fills.

OK, that was easier than I expected. I've had a look at it, and fixed those things, and also what I'm pretty sure was a pre-existing mistake in this line:

QColor selPenColor = selBrushColor == mColor ? selBrushColor : mBorderColor;

I had to tidy/rearrange the code there so I could understand it :)

I added a number of comments, mostly about inconsistencies compared with how QGIS renders polygon layers. Can someone else look at these and tell me what they think?

There is one reasonably annoying bug:

//"no fill" doesn't work if brushColor = penColor 
// e.g. if the symbol is a circle, the whole bounding box (is that what it's called?) 
// of the circle is filled.

I'm guessing to fix this an if statement is required something like the one I commented out because that method didn't work. But I don't know what to put in it!

comment:17 by Alister, 13 years ago

Ah - there's a preview button! I should use it ;)


  • fill style is not applied to selected features (actually, it looks like selected points are rendered by simply filling each symbol layer with yellow, so if transparency is applied the yellow is darker where two symbol layers overlap). I don't think this is a significant problem.
  • transparency is only applied to borders, not fills.

OK, that was easier than I expected. I've had a look at it, and fixed those things, and also what I'm pretty sure was a pre-existing mistake in this line:

QColor selPenColor = selBrushColor == mColor ? selBrushColor : mBorderColor;

I had to tidy/rearrange that section of code so I could understand it :)

I added a number of comments, mostly about inconsistencies compared with how QGIS renders polygon layers. Can someone else look at these and tell me what they think?

There is one reasonably annoying bug:

//"no fill" doesn't work if brushColor = penColor 
// e.g. if the symbol is a circle, the whole bounding box (is that what  it's called?)
// of the circle is filled.

I'm guessing to fix this an if statement is required something like the one I commented out because that method didn't work. But I don't know what to put in it!

comment:18 by Alister, 13 years ago

Oh, yeah - I noticed another problem.

In the symbol layers box with these patches it only shows the fill, whereas it used to show a preview of the symbol layer.

comment:19 by Alister, 13 years ago

"no fill" doesn't work if brushColor = penColor

e.g. if the symbol is a circle, the whole bounding box (is that what it's called?)

of the circle is filled.

Actually, this isn't quite right. It definitely happens sometimes with no fill, but I can't figure out exactly when... it looks like it is something to do with the order in which I change the settings or something odd like that.

comment:20 by Alister, 13 years ago

Oh, of course.

We shouldn't apply the fill style to selected features unless either we have a special pen colour for selected features (personally I think this would be nice), or we check to make sure the fill style isn't "no fill".

If the fill style is "no fill" and we apply it to selected features, then they look the same as unselected features :)

comment:21 by Alister, 13 years ago

Oh, and I think I was wrong about this:

// Alister - I don't understand the reasoning for the following line
//  QColor selPenColor = selBrushColor == mColor ? selBrushColor : mBorderColor;
// I think it is a mistake and is meant to be this
  QColor selPenColor = mBorderColor == mColor ? selBrushColor : mBorderColor ;

This wouldn't have been a mistake - it is trying to make the selection look different if the symbol fill is the same colour as the selection fill. But there are still things that could be improved here: it won't achieve anything if the symbol fill and the symbol outline are both the same colour as the selection fill. Also, polygon layers aren't dealt with in the same way, and they probably should be.

comment:22 by Alister, 13 years ago

Oh, yeah - I noticed another problem.

In the symbol layers box with these patches it only shows the fill, whereas it used to show a preview of the symbol layer.

Sorry, my mistake again - the preview works fine. I must have been testing with symbols larger than the space that the preview fits in.

comment:23 by Alister, 13 years ago

Please disregard my patch for now. After looking at it again I came to understand how some things were intended to work.

I have made some changes to make the display of selected features in point symbology more consistent with polygon and polyline features, but I'll check them in the next day or so before posting a new patch.

comment:24 by Alister, 13 years ago

Ah, sorry. That's a long couple of days :(

Please see the new patch. I was hoping to do more to improve the consistency between points, polylines and polygons, but I won't have a chance to do any more for a while.

Note: See TracTickets for help on using tickets.