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.

Sign up for GitHub

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

Update dimradial draw function #42

Merged
merged 1 commit into from
Apr 10, 2016
Merged

Update dimradial draw function #42

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 62 additions & 16 deletions lcviewernoqt/drawitems/lcdimradial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ void LCDimRadial::draw(LcPainter& painter, const LcDrawOptions &options, const l
// Decide to show the explecit value or the measured value
double radiusCircle = this->definitionPoint().distanceTo(this->definitionPoint2());
const lc::geo::Coordinate& mousePos = middleOfText();
const bool mouseIsInside = mousePos.distanceTo(definitionPoint()) < radiusCircle;
//const bool mouseIsInside = mousePos.distanceTo(definitionPoint()) < radiusCircle;
// FIXME this should not be fixed
const double capSize = 2.;

Expand All @@ -34,21 +34,67 @@ void LCDimRadial::draw(LcPainter& painter, const LcDrawOptions &options, const l

// Draw line
EndCaps endCaps;
auto tLinePos = !mouseIsInside
? mousePos.move(mousePos - lc::geo::Coordinate(definitionPoint().x(), mousePos.y()), te.width + capSize)
: mousePos.move(mousePos - lc::geo::Coordinate(definitionPoint().x(), mousePos.y()), -te.width - capSize);

auto tMText = !mouseIsInside
? mousePos.move(mousePos - lc::geo::Coordinate(definitionPoint().x(), mousePos.y()), te.width / 2 + capSize / 2)
: mousePos.move(mousePos - lc::geo::Coordinate(definitionPoint().x(), mousePos.y()), -te.width / 2 - capSize / 2);

painter.move_to(definitionPoint2().x(), definitionPoint2().y());
painter.line_to(mousePos.x(), mousePos.y());
painter.line_to(tLinePos.x(), mousePos.y());
painter.stroke();
endCaps.render(painter, EndCaps::CLOSEDARROW, mousePos.x(), mousePos.y(), definitionPoint2().x(), definitionPoint2().y(), capSize) ;

this->drawText(value, textAngle(), lc::TextConst::AttachmentPoint::Top_center, tMText, painter, options, rect);

const bool textisInside = middleOfText().distanceTo(mousePos) < radiusCircle;
const bool textFitsInside = radiusCircle * 2. > te.width;

if (textisInside && textFitsInside) {
auto tLinePos = (definitionPoint2().x() > mousePos.x())
? lc::geo::Coordinate(mousePos.x() - (te.width + capSize), mousePos.y())
: lc::geo::Coordinate(mousePos.x() + (te.width + capSize), mousePos.y());
auto tMText = (definitionPoint2().x() > mousePos.x())
? lc::geo::Coordinate(mousePos.x() - (te.width/2 + capSize/2), mousePos.y() - (te.width/4 + capSize/2))
: lc::geo::Coordinate(mousePos.x() + (te.width/2 + capSize/2), mousePos.y() - (te.width/4 + capSize/2));

if (definitionPoint2().y() <= mousePos.y()) {
tMText = (definitionPoint2().x() > mousePos.x())
? lc::geo::Coordinate(mousePos.x() - (te.width/2 + capSize/2), mousePos.y())
: lc::geo::Coordinate(mousePos.x() + (te.width/2 + capSize/2), mousePos.y());
}

painter.move_to(definitionPoint2().x(), definitionPoint2().y());
painter.line_to(mousePos.x(), mousePos.y());
painter.line_to(tLinePos.x(), mousePos.y());
painter.stroke();
endCaps.render(painter, EndCaps::CLOSEDARROW, mousePos.x(), mousePos.y(), definitionPoint2().x(), definitionPoint2().y(), capSize) ;

Copy link
Member

Choose a reason for hiding this comment

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

Trying to cleanup a little bit here, so the code reads easier:

    using namespace lc::geo;

    Coordinate offsetX{te.width + capSize, 0};
    Coordinate offsetY{0, te.width/4 + capSize/2};

    if (textisInside && textFitsInside) {
    auto const offset = (definitionPoint2().x() > mousePos.x()) ? - offsetX : offsetX;
    auto const tLinePos = mousePos + offset;
    auto const tMText = mousePos + offset/2;

    if  (definitionPoint2().y() > mousePos.y()) {
        tMText -= offsetY;
    }

    //the painter code might read better with some adaptive interface
    auto move_to = [&painter] (Coordinate const& coord) {painter.move_to(coord.x, coord.y);}
    auto line_to = [&painter] (Coordinate const& coord) {painter.line_to(coord.x, coord.y);}

        move_to(definitionPoint2());
        line_to(mousePos);

the logic can be further simplified as far as the intention is still clear enough to be read from code.

Copy link
Member

Choose a reason for hiding this comment

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

dli,

perhaps we can create some header for that adaptive interface with some generic (wildly guessing).
I wanted to keep our entities (Coordinate etc... etc) out of the painter functions so we don't have potential circular references, or at a minimum that the painter doesn't depend on the geo classes. This is why I didn't add the Coordinate classes as method paramaters.

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

mixing geometry/logic and dependency of painter interface seems to be bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to make such changes?

Copy link
Member

Choose a reason for hiding this comment

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

@bhattigurjot
please make your own judgement call, and avoid getting into the "deadlock" situation, as I explained early on: while one party is waiting for comments, and the other party is waiting to see actions.

Thanks!

this->drawText(value, textAngle(), lc::TextConst::AttachmentPoint::Top_center, tMText, painter, options, rect);
} else {
auto center = definitionPoint().mid(definitionPoint2());
auto p1 = definitionPoint2().moveTo(center, -capSize * 1.5);
auto p2 = definitionPoint2().moveTo(center, -(te.width + capSize));

auto tLinePos = (definitionPoint2().x() < mousePos.x())
? lc::geo::Coordinate(p2.x() - (te.width + capSize), p2.y())
: lc::geo::Coordinate(p2.x() + (te.width + capSize), p2.y());
lc::geo::Coordinate tMText;

if (definitionPoint2().x() < mousePos.x() && definitionPoint2().y() < mousePos.y()) {
// checks for 1st quad
tMText = lc::geo::Coordinate(p2.x() - (te.width/2 + capSize/2), p2.y() - (te.height + capSize));
} else if (definitionPoint2().x() >= mousePos.x() && definitionPoint2().y() < mousePos.y()) {
// checks for 2nd quad
tMText = lc::geo::Coordinate(p2.x() + (te.width/2 + capSize/2), p2.y() - (te.height + capSize));
} else if (definitionPoint2().x() > mousePos.x() && definitionPoint2().y() > mousePos.y()) {
// checks for 3rd quad
tMText = lc::geo::Coordinate(p2.x() + (te.width/2 + capSize/2), p2.y() + (te.height/4));
} else if (definitionPoint2().x() < mousePos.x() && definitionPoint2().y() >= mousePos.y()) {
// checks for 4rth quad
tMText = lc::geo::Coordinate(p2.x() - (te.width/2 + capSize/2), p2.y() + (te.height/4));
} else {
// checks for remaining cases
tMText = lc::geo::Coordinate(p2.x() + (te.width/2 + capSize/2), p2.y() + (te.height/2));
}

painter.move_to(definitionPoint().x(), definitionPoint().y());
painter.line_to(definitionPoint2().x(), definitionPoint2().y());
painter.line_to(p2.x(), p2.y());
painter.line_to(tLinePos.x(), tLinePos.y());
painter.stroke();
painter.point(p2.x(), p2.y(), 2, false);
endCaps.render(painter, EndCaps::CLOSEDARROW, p1.x(), p1.y(), definitionPoint2().x(), definitionPoint2().y(), capSize);
this->drawText(value, textAngle(), lc::TextConst::AttachmentPoint::Top_center, tMText, painter, options, rect);
}

if (modified) {
painter.restore();
Expand Down