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.

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

Renovate usages of QLineF intersections #719

Merged
merged 2 commits into from Dec 11, 2021

Conversation

YakoYakoYokuYoku
Copy link
Member

@YakoYakoYokuYoku YakoYakoYokuYoku commented Dec 8, 2021

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:

PR Description

What type of PR is this? (Check one of the boxes below)

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality nor fixes a bug but improves Natron in some way)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My change requires a change to the documentation
    • I have updated the documentation accordingly

What does this pull request do?

Both QLineF::intersect plus QLineF::IntersectType were deprecated in Qt 5.14 and QLineF::intersects plus QLineF::IntersectionType are recommended to be used instead.

Show a few screenshots (if this is a visual change)

N/A.

Have you tested your changes (if applicable)? If so, how?

Built Natron and used the node graph editor.

Futher details of this pull request

N/A.

Copy link
Member

@devernay devernay left a comment

Choose a reason for hiding this comment

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

what do you think of the alternative I propose?

@@ -4476,7 +4476,11 @@ RotoContextPrivate::bezulate(double time,
QPointF intersectionPoint;
for (; cur != polygon.end(); ++cur, ++last_pt) {
QLineF polygonSegment( QPointF(last_pt->x, last_pt->y), QPointF(cur->x, cur->y) );
#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
if (line.intersects(polygonSegment, &intersectionPoint) == QLineF::BoundedIntersection) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of having two open curly braces for one close brace in the code. Does clang-format handle this correctly?

Copy link
Member

Choose a reason for hiding this comment

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

Instead, maybe add at the beginning of each of these 4 files:

#if QT_VERSION < QT_VERSION_CHECK(5, 14, 0)
// https://doc.qt.io/qt-5/qlinef-obsolete.html
#define intersects intersect
#define IntersectionType IntersectType 
#endif

I know this is kinda dangerous if "intersects" is used in the code but this seems ok to me

Copy link
Member Author

Choose a reason for hiding this comment

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

clang-format can handle this indeed. As for the defines is better to use usings, although I prefer to not use them altogether.

Copy link
Member

Choose a reason for hiding this comment

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

actually I don't think "using" is possible, since it's inside a class, not a namespace

@YakoYakoYokuYoku YakoYakoYokuYoku force-pushed the qlinef-intersect-renov branch 2 times, most recently from 4017d92 to 8d2dc81 Compare December 11, 2021 18:26
@devernay
Copy link
Member

note that you can use auto (we build with C++11, even for Qt4)

@YakoYakoYokuYoku YakoYakoYokuYoku merged commit f5742cd into RB-2.5 Dec 11, 2021
@YakoYakoYokuYoku
Copy link
Member Author

I think that could be done in another PR, as many other parts of the codebase do this too.

@YakoYakoYokuYoku YakoYakoYokuYoku deleted the qlinef-intersect-renov branch December 11, 2021 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants