Page MenuHome

GP: Fix precision issue with Circle and Arc tools

Authored by Charlie Jolly (charlie) on Dec 4 2018, 4:21 PM.

Diff Detail

rB Blender

Event Timeline

@Antonio Vazquez (antoniov)
Here is a proposed fix. I've extended the tGPspoint struct and gp_stroke_convertcoords_tpoint function to accept float coordinates.
This is used by the fill tool too but I've not looked into that area.

Why not replace the integer coordinates by float coordinates?

Why not replace the integer coordinates by float coordinates?

tGPspoint is used all over GP so it wasn't a trivial fix to change the type, this is a pretty local fix for the primitives.

Right, I prefer a bigger change than a local fix in this case though. Exceptions like this make the code more fragile.

I agree a complete change is better than hack code, but is now the right moment to do that?

IMHO now we need to stabilize the version and not change a lot the code, if the adding of the arc tool need change the fields to float, this can affect a lot of places (including annotations), so maybe it's better wait for 2.81 to have time to run a full test and fixing any bug.

@Brecht Van Lommel (brecht)
@Antonio Vazquez (antoniov)
Reverting D4024 is understandable but that still leaves the issue with the circle tool unresolved for 2.80.
I'll upload a new diff for consideration which is less hacky :P and isolates the code from the other GP areas.

This comment was removed by Charlie Jolly (charlie).

Tidier fix for consideration.

I see that you have created a internal function to make the float conversion, so this is ok as solution for primitives. I think in Blender 2.81 we must back here and redo the primitives including a full support for Bezier curves.

This revision is now accepted and ready to land.Dec 5 2018, 4:10 PM

It's up to you guys to decide if you want to have a workaround.

Personally if I was working on this, I would just update the code to use floats. It may be tedious but not that risky if you carefully look at compiler warnings, and we're still easily 4 months away from a final release.

@Brecht Van Lommel (brecht) I've noted your comment.

@Antonio Vazquez (antoniov) I'll commit this now and look at refactoring the code to use floats.

Remove unused variable

This revision was automatically updated to reflect the committed changes.