Line drawing, tests and circles improvement #378
Conversation
Implement line drawing in images. Refs: pybricks/support#2154
Add unit tests for image drawing. Test fill and line drawing. Refs: pybricks/support#2154
Previous code was too strict on the circle radius, resulting in pointy circles. Do not touch rounded rectangles, they looks better with small radii. Refs: pybricks/support#2154
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
Very nice. I like the way you are doing the tests. It makes it very easy to visualize what is going on.
lib/pbio/src/image/image.c
Outdated
|
|
||
| /** | ||
| * Draw a line with a flat slope (less or equal to 1). | ||
| * @param [in,out] image Image to draw into. |
There was a problem hiding this comment.
| * @param [in,out] image Image to draw into. | |
| * @param [in] image Image to draw into. |
[in,out] would require pbio_image_t **image and would return a different pointer than the one that was passed in.
There was a problem hiding this comment.
I do not agree, the image is read and written, so it’s both in and out.
Random people on the internet seems to agree: https://stackoverflow.com/questions/47732665/is-that-an-in-or-in-out-parameter-doxygen-c 🙂
BTW, I find those in/out redundant for C, it’s usually clear from the function prototype.
There was a problem hiding this comment.
Sure, there is room for interpretation. I learned C# before C, so I've always used the C# semantics for in/out, which are well defined.
BTW, I find those in/out redundant for C, it’s usually clear from the function prototype.
This is exactly why I don't consider [in,out] correct here. It is only useful for something like:
int read(void* data, size_t *size);Where the precondition is that *size is set to the size of data in bytes and the post-condition is that *size is set to the number of bytes actually read, which may be less than the input size. Just going by the signature alone, one would normally expect size_t *size to be an out parameter so [in,out] makes it a bit more obvious that something unusual is going on.
Anyway, I'm not going to make a big deal about it. I don't think we've used this 100% consistently throughout the code anyway. But, in the majority of cases, we have only used [in] for cases where a struct is modified by a function. So it would be more consistent to stick with just [in].
There was a problem hiding this comment.
Sure, there is room for interpretation. I learned C# before C, so I've always used the C# semantics for in/out, which are well defined.
I guess for C# structs though, we would need to call it [in,out] like you are doing here since otherwise C# would pass the struct by value. 😄 It's just that normally something like the image type would be an object in C# rather than a struct. I consider passing a struct by reference in C like passing an object in C#. So, yeah, I get your point.
There was a problem hiding this comment.
OK, made the change for the whole file.
lib/pbio/src/image/image.c
Outdated
|
|
||
| /** | ||
| * Draw a line with a steep slope (more than 1). | ||
| * @param [in,out] image Image to draw into. |
There was a problem hiding this comment.
| * @param [in,out] image Image to draw into. | |
| * @param [in] image Image to draw into. |
And everywhere else too.
| "mp ST.......mp STAMP"); | ||
|
|
||
| test_image_prepare_images(&outer, &inner, &full); | ||
| srand(1234); |
There was a problem hiding this comment.
One thing I've done before to make tests random but also reproducible is make the seed number come from an environment variable. Then have some global init function that runs once that checks the environment variable. If it is not set, generate a random seed and print it. This way, in CI runs, we can see the seed that was used and can use the same seed locally to reproduce a failed test if needed. And it gets us a bit more coverage by actually using different numbers on each CI run.
In once case, I think I used the github CI variable for the CI run number as the seed so I didn't even have to make the global init function.
There was a problem hiding this comment.
I’m sorry, I do not agree too :-) what is the point of adding non-determinism?
If I run a CI for a given commit, I want to have the exact same result one month later with the same commit, it should not depend on external data.
About code coverage, you can check that every code path is tested. Adding non-determinism would mean that possibly a CI run would report a buggy reduced coverage for a unrelated commit.
If the global init function runs once, it means that when I select only a part of the test to run (using ./build/test-pbio src/image/.. for example), it gets different numbers. So it should at least be done at the start of each test.
Previous tests use rand without using srand so the random number generator was not initialized.
I have made the same kind of system (env variable and print) in the past, but I think it brings complexity for no gain.
Did I convince you?
There was a problem hiding this comment.
If we aren't actually going to use random data, what is the point of using rand() at runtime then? Why not just generate some random data and hard-code it in the test so that we can see what the actual input is?
The point to me of using random data is that it is too expensive to try every possible input but trying random things might eventually stumble upon some edge case we didn't consider before. If we use the same random seed every time though, we will never discover new combinations of inputs. This makes it a fuzzing test rather rather than a regression test. Both kinds of tests are useful.
There was a problem hiding this comment.
I’m not sure you would be happy to have 460000 random numbers in the source code.
If the number of loop is large enough, using a different seed in different runs brings no benefit, but brings the drawbacks I mentioned.
If I increase 10000 to 1000000, it’s the equivalent of running 100 CI, but it takes 2 seconds.
My feeling was that 10000 loops was enough to find those edge cases (and it found some edge cases that I fixed), and also, it generates nice pictures, but I can not actually prove it.
I do not want to impose anything, but I should at least try to convince you, if not, I can adapt. I hope you do not feel bad about it.
There was a problem hiding this comment.
I am convinced. (And I don't feel bad at all. 😄)
There was a problem hiding this comment.
Written form + non native language, it’s sometime easy to sound rude without having the intention. I’m happy this is not the case.
Thanks :)
|
Thanks! |
3 participants
Hi,
here are some improvements for the Image code: