-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
Add a white outline on the rubberband so it's better visible in various backgrounds #4762
Conversation
suricactus
commented
Nov 13, 2023
before | after |
---|---|
1e4a562
to
424a04c
Compare
🎉 Ta-daaa, freshly created APKs are available for 9d3697b: arm64-android |
src/core/sgrubberband.cpp
Outdated
sgGeom->setDrawingMode( QSGGeometry::DrawLineStrip ); | ||
node->setGeometry( sgGeom ); | ||
node->setMaterial( &mMaterial ); | ||
node->setFlag( QSGNode::OwnsGeometry ); | ||
node->setFlag( QSGNode::OwnedByParent ); | ||
return node; | ||
|
||
outlineSGGeom->setLineWidth( static_cast<float>( mWidth * 3 ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here I'd go for mWidth + 4 (to ever only add a 2 pixel buffer outside of the main width), I think the outline shouldn't grow in width alongside the main width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually sorry, we'll need to send the outline width separately from the rubber band class as we need the device pixel ratio to have a static outline buffer width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice when the first comment raises a question and the second one answers it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially did it like you said with extra parameter, then I thought it might not be needed to make the API slightly simpler and only add it on demand. Well, the demand came :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nirvn pushed the requested version
Nice! Could you show a screenshot of a line on a forest orthophoto? |
@suricactus , nice, thanks @m-kuhn , this is 70% towards the aimed goal, we still need coordinate cursor tweaks to match the screenshot |
@suricactus , good looks good. Seems like your build environment hasn't been hooked to the pre-commit stuff yet :) |
12c0ab3
to
4248417
Compare
4248417
to
e89f722
Compare
IMO @nirvn this look ok on various backgrounds. |
@suricactus , sweet! |
e89f722
to
f9d0e2c
Compare