Merge lp:~inkscape+alexander/inkscape/angle into lp:~inkscape.dev/inkscape/trunk

Proposed by Alexander Brock
Status: Merged
Merged at revision: 14213
Proposed branch: lp:~inkscape+alexander/inkscape/angle
Merge into: lp:~inkscape.dev/inkscape/trunk
Diff against target: 32 lines (+19/-3)
1 file modified
src/ui/tools/node-tool.cpp (+19/-3)
To merge this branch: bzr merge lp:~inkscape+alexander/inkscape/angle
Reviewer Review Type Date Requested Status
Krzysztof Kosinski Approve
Review via email: mp+261397@code.launchpad.net

Description of the change

When selecting exactly two nodes the status line shows the angle relative to the x-axis, e.g.:

"2 of 65 nodes selected, angle: 65.09°."

To post a comment you must log in.
Revision history for this message
Martin Owens (doctormo) wrote :

It would be nice if the angle calculation were split out into the more core of the code. Having it all here in the text generation for the node tool makes it a little more inflexible if we ever choose to make the selected nodes do something with the angle (for instance align, or display the angle on the screen and etc)

Can you move the core of the calculation out?

Revision history for this message
Alexander Brock (brock-alexander) wrote :

On 08/06/15 16:55, Martin Owens wrote:
> It would be nice if the angle calculation were split out into the more core of the code. Having it all here in the text generation for the node tool makes it a little more inflexible if we ever choose to make the selected nodes do something with the angle (for instance align, or display the angle on the screen and etc)
>
> Can you move the core of the calculation out?

I don't exactly know where the core of Inkscape is, in which file should
the calculation be done?
Which part should be moved to the core exactly? Should I write a
function which takes two Geom::Point and returns the angle in ° like that?

double getAngle(const Geom::Point& a, const Geom::Point& b) {
   const Geom::Point difference = b-a;
   double angle = Geom::atan2(difference);
   if (angle < 0) {
         angle += 2*M_PI;
   }
   angle *= 180.0/M_PI;
}

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

What you actually want to compute is the angle between a line going from a to b and the X axis. Therefore, you should use Geom::rad_to_deg(Geom::Line(a, b).angle()).

Also please read this document, because your variable names do not match coding style.
https://inkscape.org/en/develop/coding-style/

review: Needs Fixing
14197. By Alexander Brock

Compute angle in [0,180[ and adhere to style guide

14198. By Alexander Brock

Change function call to one line

Revision history for this message
Alexander Brock (brock-alexander) wrote :

> What you actually want to compute is the angle between a line going from a to
> b and the X axis. Therefore, you should use Geom::rad_to_deg(Geom::Line(a,
> b).angle()).

Makes sense, I changed that.

> Also please read this document, because your variable names do not match
> coding style.
> https://inkscape.org/en/develop/coding-style/

I changed it, better now?

Revision history for this message
Alexander Brock (brock-alexander) wrote :

I'd like to gently nudge someone to comment / merge :-)

Revision history for this message
Krzysztof Kosinski (tweenk) wrote :

Looks okay now, merged.

review: Approve
Revision history for this message
Alexander Brock (brock-alexander) wrote :

On 24/06/15 22:56, Krzysztof Kosinski wrote:
> Looks okay now, merged.

Thank you very much :-)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/ui/tools/node-tool.cpp'
--- src/ui/tools/node-tool.cpp 2015-05-30 18:27:42 +0000
+++ src/ui/tools/node-tool.cpp 2015-06-10 00:56:17 +0000
@@ -665,9 +665,25 @@
665 unsigned total = this->_selected_nodes->allPoints().size();665 unsigned total = this->_selected_nodes->allPoints().size();
666666
667 if (sz != 0) {667 if (sz != 0) {
668 char *nodestring = g_strdup_printf(668 char *nodestring;
669 ngettext("<b>%u of %u</b> node selected.", "<b>%u of %u</b> nodes selected.", total),669 if (sz == 2) {
670 sz, total);670 Inkscape::UI::ControlPointSelection::Set &selection_nodes = this->_selected_nodes->allPoints();
671 std::vector<Geom::Point> positions;
672 for (Inkscape::UI::ControlPointSelection::Set::iterator i = selection_nodes.begin(); i != selection_nodes.end(); ++i) {
673 if ((*i)->selected()) {
674 Inkscape::UI::Node *n = dynamic_cast<Inkscape::UI::Node *>(*i);
675 positions.push_back(n->position());
676 }
677 }
678 g_assert(positions.size() == 2);
679 const double angle = Geom::rad_to_deg(Geom::Line(positions[0], positions[1]).angle());
680 nodestring = g_strdup_printf("<b>%u of %u</b> nodes selected, angle: %.2f°.", sz, total, angle);
681 }
682 else {
683 nodestring = g_strdup_printf(
684 ngettext("<b>%u of %u</b> node selected.", "<b>%u of %u</b> nodes selected.", total),
685 sz, total);
686 }
671687
672 if (this->_last_over) {688 if (this->_last_over) {
673 // TRANSLATORS: The %s below is where the "%u of %u nodes selected" sentence gets put689 // TRANSLATORS: The %s below is where the "%u of %u nodes selected" sentence gets put