Chlumsky / msdfgen

Multi-channel signed distance field generator
MIT License
3.9k stars 404 forks source link

Div by zero fixes #204

Open GlassBeaver opened 3 months ago

GlassBeaver commented 3 months ago

Fixed three cases of division by zero.

You can use the following macro to get errors at runtime when a div by zero or other floating-point error occurs, like NaN or inf. Need to enable it on every thread individually at thread creation (for a single-threaded app, just put it first thing in the main function). Ideally, you put this inside #if _DEBUG

// throw floating-point exceptions on any errors and NaN or inf

define THROW_ON_FP_ERROR() \

uint32_t origFP; \
_controlfp_s(&origFP, 0, 0); \
_controlfp_s(nullptr, origFP & ~(_EM_INVALID | _EM_ZERODIVIDE), _MCW_EM);
Chlumsky commented 3 months ago

What is wrong with division by zero? Subsequent conditions will detect inf & nan values, so I don't see the issue. Do you have any examples where this fixes a wrong result?

GlassBeaver commented 3 months ago

In your particular functions, you might be able to get away with it but others using your library might be looking out for div by zero and other floating-point errors for various reasons. See above for the macro. In such a case, their programs won't get past the erroneous parts.

I'm not sure I understand why it's a problem to fix the div by zero errors right at the source.

If you're worried about performance, the branch predictor has a very good chance of predicting the additional branches (you can verify that in Intel vTune or AMD uProf) and if there are lots of div by zeroes then you're actually saving performance by doing an early exit, most notably in hasDiagonalArtifact() since you would skip all the calls to hasDiagonalArtifactInner()

Chlumsky commented 3 months ago

Because I don't see floating-point division by zero as an error to be fixed, it's just something that can happen.

GlassBeaver commented 3 months ago

See my explanation above for why it's considered an error. Fixing it doesn't hurt the codebase, on the contrary it actually improves it. See above for the performance-related part if you're concerned about performance. If you're offended by my fix for whatever reason then reject the PR and be done with it. It's certainly not worth it to argue about this.

Chlumsky commented 3 months ago

I am not offended or concerned about performance, I am simply unconvinced by your argument.