Skip to content

Commit

Permalink
Fix confusion in angle handling for label results, where angles
Browse files Browse the repository at this point in the history
in radians were being treated as degrees

This made an interactive movement of a label shown for a line
segment initially have a quasi-random rotation, instead of
keeping the rotation of the original label
  • Loading branch information
nyalldawson committed Jul 28, 2023
1 parent a4d131c commit fbb9e08
Show file tree
Hide file tree
Showing 10 changed files with 63 additions and 45 deletions.
2 changes: 1 addition & 1 deletion src/app/labeling/qgsmaptoollabel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ bool QgsMapToolLabel::currentLabelRotationPoint( QgsPointXY &pos, bool ignoreUps
break;
}

double angle = mCurrentLabel.pos.rotation;
double angle = mCurrentLabel.pos.rotation * M_PI / 180;
double xd = xdiff * std::cos( angle ) - ydiff * std::sin( angle );
double yd = xdiff * std::sin( angle ) + ydiff * std::cos( angle );
if ( mCurrentLabel.pos.upsideDown && !ignoreUpsideDown )
Expand Down
2 changes: 1 addition & 1 deletion src/app/labeling/qgsmaptoolmovelabel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ void QgsMapToolMoveLabel::cadCanvasPressEvent( QgsMapMouseEvent *e )
int rCol;
if ( currentLabelDataDefinedRotation( defRot, rSuccess, rCol ) )
{
const double labelRot = mCurrentLabel.pos.rotation * 180 / M_PI;
const double labelRot = mCurrentLabel.pos.rotation;
vlayer->changeAttributeValue( mCurrentLabel.pos.featureId, rCol, labelRot );
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/app/labeling/qgsmaptoolpinlabels.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ bool QgsMapToolPinLabels::pinUnpinCurrentLabel( bool pin )

double labelX = referencePoint.x();
double labelY = referencePoint.y();
double labelR = labelpos.rotation * 180 / M_PI;
double labelR = labelpos.rotation;

// transform back to layer crs
QgsPointXY transformedPoint = mCanvas->mapSettings().mapToLayerCoordinates( vlayer, referencePoint );
Expand Down
18 changes: 15 additions & 3 deletions src/core/labeling/qgslabelfeature.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,11 @@ class CORE_EXPORT QgsLabelFeature
//TODO - remove when QgsGeometry caches GEOS preparedness
const GEOSPreparedGeometry *permissibleZonePrepared() const { return mPermissibleZoneGeosPrepared.get(); }

//! Size of the label (in map units)
/**
* Size of the label (in map units).
*
* An optional \a angle (in radians) can be specified to return the size taking into account the rotation.
*/
QSizeF size( double angle = 0.0 ) const;

/**
Expand Down Expand Up @@ -227,9 +231,17 @@ class CORE_EXPORT QgsLabelFeature
bool hasFixedAngle() const { return mHasFixedAngle; }
//! Sets whether the label should use a fixed angle instead of using angle from automatic placement
void setHasFixedAngle( bool enabled ) { mHasFixedAngle = enabled; }
//! Angle in degrees of the fixed angle (relevant only if hasFixedAngle() returns TRUE)

/**
* Angle in radians of the fixed angle (relevant only if hasFixedAngle() returns TRUE)
* \see setFixedAngle()
*/
double fixedAngle() const { return mFixedAngle; }
//! Sets angle in degrees of the fixed angle (relevant only if hasFixedAngle() returns TRUE)

/**
* Sets the \a angle in radians of the fixed angle (relevant only if hasFixedAngle() returns TRUE).
* \see fixedAngle()
*/
void setFixedAngle( double angle ) { mFixedAngle = angle; }

/**
Expand Down
2 changes: 1 addition & 1 deletion src/core/labeling/qgslabelsearchtree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ bool QgsLabelSearchTree::insertLabel( pal::LabelPosition *labelPos, QgsFeatureId

const QgsRectangle bounds( xMin, yMin, xMax, yMax );
const QgsGeometry labelGeometry( QgsGeometry::fromPolygonXY( QVector<QgsPolylineXY>() << cornerPoints ) );
std::unique_ptr< QgsLabelPosition > newEntry = std::make_unique< QgsLabelPosition >( featureId, labelPos->getAlpha() + mMapSettings.rotation(), cornerPoints, bounds,
std::unique_ptr< QgsLabelPosition > newEntry = std::make_unique< QgsLabelPosition >( featureId, -labelPos->getAlpha() * 180 / M_PI + mMapSettings.rotation(), cornerPoints, bounds,
labelPos->getWidth(), labelPos->getHeight(), layerName, labeltext, labelfont, labelPos->getUpsideDown(), diagram, pinned, providerId, labelGeometry, isUnplaced );
newEntry->groupedLabelId = uniqueLinkedId;
mSpatialIndex.insert( newEntry.get(), bounds );
Expand Down
17 changes: 9 additions & 8 deletions src/core/labeling/qgspallabeling.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2344,7 +2344,8 @@ std::unique_ptr<QgsLabelFeature> QgsPalLayerSettings::registerFeatureWithDetails
//data defined position / alignment / rotation?
bool layerDefinedRotation = false;
bool dataDefinedRotation = false;
double xPos = 0.0, yPos = 0.0, angle = 0.0;
double xPos = 0.0, yPos = 0.0;
double angleInRadians = 0.0;
double quadOffsetX = 0.0, quadOffsetY = 0.0;
double offsetX = 0.0, offsetY = 0.0;
QgsPointXY anchorPosition;
Expand Down Expand Up @@ -2461,7 +2462,7 @@ std::unique_ptr<QgsLabelFeature> QgsPalLayerSettings::registerFeatureWithDetails
if ( !qgsDoubleNear( angleOffset, 0.0 ) )
{
layerDefinedRotation = true;
angle = ( 360 - angleOffset ) * M_PI / 180; // convert to radians counterclockwise
angleInRadians = ( 360 - angleOffset ) * M_PI / 180; // convert to radians counterclockwise
}

const QgsMapToPixel &m2p = context.mapToPixel();
Expand All @@ -2484,7 +2485,7 @@ std::unique_ptr<QgsLabelFeature> QgsPalLayerSettings::registerFeatureWithDetails
// TODO: add setting to disable having data defined rotation follow
// map rotation ?
rotationDegrees += m2p.mapRotation();
angle = ( 360 - rotationDegrees ) * M_PI / 180.0;
angleInRadians = ( 360 - rotationDegrees ) * M_PI / 180.0;
}
}
}
Expand Down Expand Up @@ -2550,7 +2551,7 @@ std::unique_ptr<QgsLabelFeature> QgsPalLayerSettings::registerFeatureWithDetails
// layer rotation set, but don't rotate pinned labels unless data defined
if ( layerDefinedRotation && !dataDefinedRotation )
{
angle = 0.0;
angleInRadians = 0.0;
}

//horizontal alignment
Expand Down Expand Up @@ -2608,8 +2609,8 @@ std::unique_ptr<QgsLabelFeature> QgsPalLayerSettings::registerFeatureWithDetails
if ( dataDefinedRotation )
{
//adjust xdiff and ydiff because the hali/vali point needs to be the rotation center
double xd = xdiff * std::cos( angle ) - ydiff * std::sin( angle );
double yd = xdiff * std::sin( angle ) + ydiff * std::cos( angle );
double xd = xdiff * std::cos( angleInRadians ) - ydiff * std::sin( angleInRadians );
double yd = xdiff * std::sin( angleInRadians ) + ydiff * std::cos( angleInRadians );
xdiff = xd;
ydiff = yd;
}
Expand Down Expand Up @@ -2725,8 +2726,8 @@ std::unique_ptr<QgsLabelFeature> QgsPalLayerSettings::registerFeatureWithDetails
labelFeature->setHasFixedPosition( hasDataDefinedPosition );
labelFeature->setFixedPosition( QgsPointXY( xPos, yPos ) );
// use layer-level defined rotation, but not if position fixed
labelFeature->setHasFixedAngle( dataDefinedRotation || ( !hasDataDefinedPosition && !qgsDoubleNear( angle, 0.0 ) ) );
labelFeature->setFixedAngle( angle );
labelFeature->setHasFixedAngle( dataDefinedRotation || ( !hasDataDefinedPosition && !qgsDoubleNear( angleInRadians, 0.0 ) ) );
labelFeature->setFixedAngle( angleInRadians );
labelFeature->setQuadOffset( QPointF( quadOffsetX, quadOffsetY ) );
labelFeature->setPositionOffset( QgsPointXY( offsetX, offsetY ) );
labelFeature->setOffsetType( offsetType );
Expand Down
16 changes: 8 additions & 8 deletions src/core/pal/feature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2146,23 +2146,23 @@ std::size_t FeaturePart::createCandidatesOutsidePolygon( std::vector<std::unique
std::vector< std::unique_ptr< LabelPosition > > FeaturePart::createCandidates( Pal *pal )
{
std::vector< std::unique_ptr< LabelPosition > > lPos;
double angle = mLF->hasFixedAngle() ? mLF->fixedAngle() : 0.0;
double angleInRadians = mLF->hasFixedAngle() ? mLF->fixedAngle() : 0.0;

if ( mLF->hasFixedPosition() )
{
lPos.emplace_back( std::make_unique< LabelPosition> ( 0, mLF->fixedPosition().x(), mLF->fixedPosition().y(), getLabelWidth( angle ), getLabelHeight( angle ), angle, 0.0, this, false, LabelPosition::Quadrant::QuadrantOver ) );
lPos.emplace_back( std::make_unique< LabelPosition> ( 0, mLF->fixedPosition().x(), mLF->fixedPosition().y(), getLabelWidth( angleInRadians ), getLabelHeight( angleInRadians ), angleInRadians, 0.0, this, false, LabelPosition::Quadrant::QuadrantOver ) );
}
else
{
switch ( type )
{
case GEOS_POINT:
if ( mLF->layer()->arrangement() == Qgis::LabelPlacement::OrderedPositionsAroundPoint )
createCandidatesAtOrderedPositionsOverPoint( x[0], y[0], lPos, angle );
createCandidatesAtOrderedPositionsOverPoint( x[0], y[0], lPos, angleInRadians );
else if ( mLF->layer()->arrangement() == Qgis::LabelPlacement::OverPoint || mLF->hasFixedQuadrant() )
createCandidatesOverPoint( x[0], y[0], lPos, angle );
createCandidatesOverPoint( x[0], y[0], lPos, angleInRadians );
else
createCandidatesAroundPoint( x[0], y[0], lPos, angle );
createCandidatesAroundPoint( x[0], y[0], lPos, angleInRadians );
break;

case GEOS_LINESTRING:
Expand Down Expand Up @@ -2206,15 +2206,15 @@ std::vector< std::unique_ptr< LabelPosition > > FeaturePart::createCandidates( P
double cx, cy;
getCentroid( cx, cy, mLF->layer()->centroidInside() );
if ( qgsDoubleNear( mLF->distLabel(), 0.0 ) )
created += createCandidateCenteredOverPoint( cx, cy, lPos, angle );
created += createCandidatesAroundPoint( cx, cy, lPos, angle );
created += createCandidateCenteredOverPoint( cx, cy, lPos, angleInRadians );
created += createCandidatesAroundPoint( cx, cy, lPos, angleInRadians );
break;
}
case Qgis::LabelPlacement::OverPoint:
{
double cx, cy;
getCentroid( cx, cy, mLF->layer()->centroidInside() );
created += createCandidatesOverPoint( cx, cy, lPos, angle );
created += createCandidatesOverPoint( cx, cy, lPos, angleInRadians );
break;
}
case Qgis::LabelPlacement::Line:
Expand Down
6 changes: 2 additions & 4 deletions src/core/pal/feature.h
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,12 @@ namespace pal
bool hasSameLabelFeatureAs( FeaturePart *part ) const;

/**
* Returns the width of the label, optionally taking an \a angle into account.
* \returns the width of the label
* Returns the width of the label, optionally taking an \a angle (in radians) into account.
*/
double getLabelWidth( double angle = 0.0 ) const { return mLF->size( angle ).width(); }

/**
* Returns the height of the label, optionally taking an \a angle into account.
* \returns the hieght of the label
* Returns the height of the label, optionally taking an \a angle (in radians) into account.
*/
double getLabelHeight( double angle = 0.0 ) const { return mLF->size( angle ).height(); }

Expand Down
7 changes: 5 additions & 2 deletions src/core/pal/labelposition.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ namespace pal
* \param y1 down-left y coordinate
* \param w label width
* \param h label height
* \param alpha rotation in rad
* \param alpha rotation in radians
* \param cost geographic cost
* \param feature labelpos owners
* \param isReversed label is reversed
Expand Down Expand Up @@ -270,9 +270,10 @@ namespace pal
double getHeight() const { return h; }

/**
* Returns the angle to rotate text (in rad).
* Returns the angle to rotate text (in radians).
*/
double getAlpha() const;

bool getReversed() const { return reversed; }
bool getUpsideDown() const { return upsideDown; }

Expand Down Expand Up @@ -368,7 +369,9 @@ namespace pal

int nbOverlap;

//! Rotation in radians
double alpha;

double w;
double h;

Expand Down
36 changes: 20 additions & 16 deletions tests/src/core/testqgslabelingengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3801,6 +3801,7 @@ void TestQgsLabelingEngine::labelingResults()
settings.isExpression = true;
settings.placement = Qgis::LabelPlacement::OverPoint;
settings.priority = 10;
settings.angleOffset = 3;

std::unique_ptr< QgsVectorLayer> vl2( new QgsVectorLayer( QStringLiteral( "Point?crs=epsg:4326&field=id:integer" ), QStringLiteral( "vl" ), QStringLiteral( "memory" ) ) );
vl2->setRenderer( new QgsNullSymbolRenderer() );
Expand Down Expand Up @@ -3865,11 +3866,11 @@ void TestQgsLabelingEngine::labelingResults()
QCOMPARE( labels.at( 0 ).labelText, QStringLiteral( "1" ) );
QGSCOMPARENEAR( labels.at( 0 ).width, 167961, 500 ); // tolerance will probably need tweaking, to account for cross-platform font diffs
QGSCOMPARENEAR( labels.at( 0 ).height, 295119, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMinimum(), -779822, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMaximum(), -611861, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMinimum(), 6897647, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMaximum(), 7192767, 500 );
QCOMPARE( labels.at( 0 ).rotation, 0.0 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMinimum(), -787429, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMaximum(), -604253, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMinimum(), 6893454, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMaximum(), 7196960, 500 );
QCOMPARE( labels.at( 0 ).rotation, -357 );

labels = results->labelsAtPosition( QgsPointXY( -769822, 6927647 ) );
QCOMPARE( labels.count(), 1 );
Expand All @@ -3884,22 +3885,22 @@ void TestQgsLabelingEngine::labelingResults()
QCOMPARE( labels.at( 0 ).labelText, QStringLiteral( "8888" ) );
QGSCOMPARENEAR( labels.at( 0 ).width, 671844, 500 ); // tolerance will probably need tweaking, to account for cross-platform font diffs
QGSCOMPARENEAR( labels.at( 0 ).height, 295119, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMinimum(), -2779386, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMaximum(), -2107542, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMinimum(), 9240403, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMaximum(), 9535523, 500 );
QCOMPARE( labels.at( 0 ).rotation, 0.0 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMinimum(), -2786649, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMaximum(), -2100279, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMinimum(), 9223025, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMaximum(), 9552902, 500 );
QCOMPARE( labels.at( 0 ).rotation, -357 );
labels = results->labelsAtPosition( QgsPointXY( -1383, 6708478 ) );
QCOMPARE( labels.count(), 1 );
QCOMPARE( labels.at( 0 ).featureId, 3 );
QCOMPARE( labels.at( 0 ).labelText, QStringLiteral( "33333" ) );
QGSCOMPARENEAR( labels.at( 0 ).width, 839805, 500 ); // tolerance will probably need tweaking, to account for cross-platform font diffs
QGSCOMPARENEAR( labels.at( 0 ).height, 295119, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMinimum(), -433112, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMaximum(), 406692, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMinimum(), 6563006, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMaximum(), 6858125, 500 );
QCOMPARE( labels.at( 0 ).rotation, 0.0 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMinimum(), -440260, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.xMaximum(), 413840, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMinimum(), 6541232, 500 );
QGSCOMPARENEAR( labels.at( 0 ).labelRect.yMaximum(), 6879899, 500 );
QCOMPARE( labels.at( 0 ).rotation, -357 );
labels = results->labelsAtPosition( QgsPointXY( -2463392, 6708478 ) );
QCOMPARE( labels.count(), 0 );

Expand Down Expand Up @@ -3938,7 +3939,10 @@ void TestQgsLabelingEngine::labelingResults()

mapSettings.setLayers( {vl2.get() } );

// with rotation
// with map rotation
settings.angleOffset = 0;
vl2->setLabeling( new QgsVectorLayerSimpleLabeling( settings ) ); // TODO: this should not be necessary!
vl2->setLabelsEnabled( true );
mapSettings.setRotation( 60 );
QgsMapRendererSequentialJob job2( mapSettings );
job2.start();
Expand Down

0 comments on commit fbb9e08

Please sign in to comment.