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

New methods "drawSmoothPolygon" and "fillSmoothPolygon". #3145

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Tom1304
Copy link

@Tom1304 Tom1304 commented Jan 31, 2024

Hi bodmer,
I wrote two little functions for your amazing great library, "drawSmoothPolygon" and "fillSmoothPolygon".
Maybe you find it worth to integrate it into your master branch.
I originally made them for displaying a wind direction arrow.
arrow

If you have any questions, comments or suggestions for improvement, please let me know.
Tom


//--------------------------------------------------------------------------------
// Rotation in degrees (0..360)
void TFT_eSPI::fillSmoothPolygon(tFPoints aPoints, float aLineWidth, uint32_t aLineColor,
Copy link

Choose a reason for hiding this comment

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

Consider passing aPoints by const reference to avoid copying the structure.

Copy link
Author

Choose a reason for hiding this comment

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

Hi B3rn475,
thanks for your comment. I just changed the code considering your suggestions.

Copy link

Choose a reason for hiding this comment

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

See other comment.


//--------------------------------------------------------------------------------
// Rotation in degrees (0..360)
void TFT_eSPI::drawSmoothPolygon(tFPoints aPoints, float aLineWidth, uint32_t aLineColor,
Copy link

Choose a reason for hiding this comment

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

Consider passing aPoints by const reference to avoid copying the structure.

Copy link
Author

Choose a reason for hiding this comment

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

Hi B3rn475,
thanks for your comment. I just changed the code considering your suggestions.

Copy link

Choose a reason for hiding this comment

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

It should be const tPoints&.


//--------------------------------------------------------------------------------
// Calculates closest distance of point to a vector (with origin at 0,0)
float TFT_eSPI::calcDistance(tFPoint *aPoint, tFPoint *aVector) {
Copy link

Choose a reason for hiding this comment

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

As you are not mutating the arguments, consider passing them by value or by const reference.

Copy link
Author

Choose a reason for hiding this comment

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

Hi B3rn475,
isn't a * parameter as good as a "pass by reference"?

Copy link

Choose a reason for hiding this comment

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

Depends on the definition of "good" ;-)

From a performance standpoint the compiler will probably generate the same code.

From a readability/usability standpoint there are some big difference.

  • With a const ref you give the guarantee to the caller that you are not going to mutate the content, and you get the guarantee that they are always going to pass something. Information that the compiler could also use in some cases.
  • With a non-const pointer you give no guarantee that you will not mutate it. Also, you have no guarantee that they will not pass nullptr.

const ref also has the ability to access temporaries, while a pointer can't.

In general a common style guide is.

  • const ref means "I always want this argument, and I will not mutate it. Temporaries are OK".
  • non-const ref means "I always want this argument, and I may mutate it. No temporaries".
  • const pointer means "the argument is optional, and I will not mutate it".
  • non-const pointer means "the argument is optional, and I may mutate it".

There are variations, i.e. some style guide prefer non-const pointer to non-const ref, but that is the general idea.

Copy link
Author

Choose a reason for hiding this comment

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

Great, thanks, perfect tutorial!
That way I never stop learning from the Pro's 8-)

Would you please have a look at my latest commit, is it the way you would do it?
Or did I still misunderstand anything

Would you even mark all the other "simple" parameters, like float aLineWidth as const float aLineWidth if they are not changed in the function?

Copy link

Choose a reason for hiding this comment

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

Before going in the "should I prefer const ref to value" topic, here is an interesting thing.

Marking a parameter passed by value as const in a function declaration has no actual impact.
void DoSomething(Foo foo) is equivalent to void DoSomething(const Foo foo).
Note that I'm not saying "they are logically equivalent", they actually generate the same signature.

Where const, for parameters passed by value, has an impact is function definition. But the impact is only safety and documentation. You are telling the compiler "do not let me mutate this value".

Here is a fun example.

int sum(int a, int b); // no const here

int main() {
    std::cout << sum(1, 2);
    return 0;
}

int sum(const int a, const int b) { // const here
    return a + b;
}

Going back to the "should I prefer to pass everything as const ref instead of by value if I don't want to mutate it".

The answer is a classic in IT... it depends.
Passing by value triggers a copy, so anything that is expensive to copy is better to pass by const reference.
A common definition of "expensive to copy" is "anything that has a complex copy constructor and anything that is bigger than a pointer".
A common rule of thumb is:

  • Pass "Fundamental types" (e.g. bool, int, float, ...) by value, as it costs the same as passing a pointer.
  • Pass classes or structs by const ref.

If you "go deeper" there is some cost associated with accessing an object via a reference, or a pointer, as there is probably going to be an indirection.
In the examples of calcDistance and checkIntersection, if you take into consideration the cost of accessing the field via a pointer or a reference, there is a chance that pasing by value (and paying for the copy of the struct) could be compensated by avoiding the need of accessing its fields indirectly.

N.B. We are talking about micro-optimizations. So prefer to pass anything that is "obviously expensive to copy" (e.g. std::vector) by const reference and for smaller object either run a benchmark or go for the simpler/safer solution.

In general you should point first at readability/ergonomics, e.g. rotatePoint could be a bit easier to understand if it had the signature tFPoint rotatePoint(tFPoint point, float angle), as you would use it as rotated = rotatePoint(original, angle), which is a bit easier to follow than rotatePoint(point_that_is_going_to_be_mutated, angle), and can be invoked even with const arguments.

The general advice is "make the API easy to understand/use and avoid unexpected side-effects".
An interesting talk about this https://youtu.be/3WBaY61c9sE (jumping to the focus point https://youtu.be/3WBaY61c9sE?t=2601)

Copy link
Author

Choose a reason for hiding this comment

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

THANKS!!!!
I think I now got a better understanding of all that parameter stuff.
Even if you label it as "micro-optimizations" it seems to give me a look inside good styling of code.
Thanks again for all your patient explanations and having a closer look at my code.
Tom

// 0: No intersection
// 1: Intersection with ascending vector
// -1: Intersection with descending vector
int TFT_eSPI::checkIntersection(tFPoint *aPoint, tFPoint *aVector) {
Copy link

Choose a reason for hiding this comment

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

As you are not mutating the arguments consider passing them by value or const reference.

Copy link
Author

Choose a reason for hiding this comment

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

Hi B3rn475,
isn't a * parameter as good as a "pass by reference"?

Copy link

Choose a reason for hiding this comment

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

Se other comment.

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