Removing returned errors from shape generation functions

soypat commented 2 years ago

Background

I had previously mentioned in this issue:

An error on a function like Cylinder3D can only be handled one way really: correcting the argument to it in the source code as one generates the shape! This is even implied with the implementation of the ErrMsg function: it includes the line number of the function that yielded the error. panic already does that and saves us having to formally handle the error message.

@deadsy has mentioned the reason for this

It was done this way because: [...] I wanted to support the programmatic generation of shapes (E.g. evolutionary algorithms) and that implied bad parameter values needed a non-panic response.

I initially agreed it was desirable that functions do not panic but I have now rolled back on this opinion. The reason being most users (+99% I'd guess) will suffer from this decision needlessly. My argument in this issue is that we can get the best of both worlds without compromising on robustness of an SDF program which requires this strict error handling. Let me explain.

Proposal

I propose changing

func Cylinder(radius, round float64) (SDF3, error)

to

func Cylinder(radius, round float64) SDF3 // panic on error

It is proposed that:

incomplete/impossible geometry parameters cause a panic.
No parameter correcting be done under the covers

I argue most users today do not make use of error handling of sdfx errors and this will be a welcome change.

Impact

Pros

Will positively impact the sdfx user developer experience
Simplify internal implementations greatly.
More familiar usage for people coming from simpler languages

Cons

Eliminate sdf error handling

Impact mitigation

If error handling were required then wrapper functions can be declared in the future with little effort. See this example: https://go.dev/play/p/lMZnaJV7o4Y

deadsy commented 2 years ago

s, err := sdf.Shape()

or ...

s, _ := sdf.Shape()

which will panic as soon as you try to use the nil value for s.

There's nothing stopping you from ignoring the errors if that's the way you roll.

soypat commented 2 years ago

There's nothing stopping you from ignoring the errors if that's the way you roll.

This is definitely not what I'm trying to get at. Errors are great, I want errors.

What I'm trying to get at is changing the current API to reduce the amount of programming users have to shell out to handle errors. I imagine most users, if not all, (certainly the sdfx examples do it this way) create shapes and stop program execution on an error and print the error. Why not save the user all this boilerplate?

s, err := sdf.Shape()
if err != nil {
    panic(err)
}

becomes

s := sdf.Shape()

with virtually no change in functionality of the library (as far as I've seen it been used). If real error handling were to be needed it can be done though I doubt this will be the case, modern evolutionary algorithm libraries handle constraints on values and even support panic during execution as a natural form of error handling (one can never be 100% sure a simulation will not panic).

It'd be great to get some feedback from sdf users to see if this sentiment is shared by the majority. So far I've had a couple people approach me in the last two days interested in the new sdf rewrite I'm taking on, so at least I know I'm not alone in this sentiment.

deadsy commented 2 years ago

https://go.dev/blog/defer-panic-and-recover

The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values.

Appeals to authority aside, I don't want to write a fragile library that crashes with invalid user input, I want it to return an error with some data telling them exactly what their problem is. The user then gets to work out how they want to treat that error, rather than the library causing a panic.

soypat commented 2 years ago

Appeals to authority aside, I don't want to write a fragile library that crashes with invalid user input, I want it to return an error with some data telling them exactly what their problem is.

Fair enough. I may not agree with your posture but I respect it. I will close this issue.

deadsy commented 2 years ago

Thanks. One thing I would like to see is a better error message which gave the call stack leading up to the failure. Often times when you bubble the error message up it's hard to know where the issue was caused. Of course a panic gives you that but I think there are some error wrap functions that can do the same thing.

stevegt commented 2 years ago

@soypat The original sdfx code did not return errors but instead called panic() in core functions. My own team and I worked to convince @deadsy to let us convert those panics to err returns, and then we did most of the work to make that conversion -- you'll see it in PRs #26 and #27. This meant that, yes, fluent style went away, much to Jason's own chagrin. So go easy on Jason; it wasn't his idea in the first place and we twisted his arm. ;-)

I'm glad you closed this issue -- it would have been sad to undo all that work we did, and it's easier and more readable to wrap those err returns in fluent-style functions for ease of use than it is to recover() from core panics. (See the goadapt package I maintain below -- I rely on recover() a bunch in there; its urgly.)

In my own case, I tend to carry around a little bit of code that wraps the few shapes I want to use fluent style for -- it's so small that I haven't sent Jason a PR with it. If I had seen this issue earlier I would have recommended that you just provide a more complete wrapper, and I see you're going that direction with the must* packages in sdf. I also like the STL import work you did over there -- I just now stumbled across it, saw your comments in the README about the err returns, and came here to correct the record and clear Jason's good name.

As an aside, @deadsy you might be able to mine some of the error stack wrap/unwrap code from github.com/stevegt/goadapt or from one of the packages it refers to in comments. The package is a bit feature-rich, but the gist of it is in the Return() and Ck() functions:

import . "github.com/stevegt/goadapt"    

func foo() (err error) {              
    defer Return(&err) // optional -- with this we wrap and return the err, without this we panic in Ck()    
    err = somethingBad()                                                                               
    Ck(err)                                                                                         
    return                                                                                          
}

deadsy commented 2 years ago

Another possible approach is to split the creation of the object and the validation checking of the objects into separate methods on the interface. ie - the creation doesn't return an error and you can compose them together without messing up your code with any error checking. When you go to call the renderer (or whatever is using the sdf) then you call the top-level validation (and validation on sub objects) which will give you back an error before actually trying to do the render.

ie - best of both worlds, easy assembly of objects, rigorous validation at point of consumption.

soypat commented 2 years ago

@stevegt Yeah, a lot has happened since the creation of this issue. I'm now convinced there is merit in an error handled API. I still stand behind what I said about needing a non-error handled API though, that is particularly important to me when I design parts since error handling can make things just so very verbose to do things you know won't fail, and even if they do the project is small enough that looking for the bug is not really an issue (or even easier since panic ~messages give you stack trace~ keep the stack when debugging with breakpoint in panic function).

Anyways, I'll be out of your hair since I've split and am now using another library.

PS. Not sure but validation at point of consumption sounds like a bad idea

stevegt commented 2 years ago

Just adding a note for future travelers that we discussed the err/panic decisions and alternatives including panic/recover in excruciating detail in #24 -- I'd even forgotten how much we documented there.

stevegt commented 2 years ago

In my own case, I tend to carry around a little bit of code that wraps the few shapes I want to use fluent style for -- it's so small that I haven't sent Jason a PR with it.

Turns out that while I didn't send Jason a PR with that fluent-style wrapper, I did push it to a repo -- it's at https://github.com/stevegt/sdfxshape. It's tiny, not only because it's a simple job to wrap those errs but also 'cause there's not much there yet beyond basic shapes. I do expect it to grow over time.

deadsy / sdfx