fixing panics

[stevegt]

Hey Jason, just wanted to let you know that @koshchei is fixing all of the 50+ panics right now throughout the code base, and @Ryan-pelo is finding and fixing the few places where there are errors being printed without an actual err return. We've been working together on our own in-house video calls on this over the last couple of days. I'd expect the panics pull request to be ready for you by Monday or Tuesday.

@koshchei has pulled your spiral commits so he already has those panic fixes now -- just in the nick of time; he was about to fix spiral next. ;-)

[deadsy]

Well. Ok. I'm a lazy optimal kind of guy. I guess I'll just wait for the PRs to roll in. :-)

[stevegt]

This is pretty much done. Only outstanding items I know of have to do with method chaining -- we may have been too aggressive and may have broken method chaining ability in some spots. Note to self and @koshchei : See conversation in #27, and go through ./examples/ looking for places where we turned inlines or method chains into several lines. If the method being called is public and returns the same type of struct that the method is attached to, and if the method returned only that type as of 32a8630, then we likely need to revert and/or find another way to ensure method chaining can still be used as a calling style.

One possibility is the two-level wrapper/core compromise mentioned in #27 -- something like the following, which would allow callers to choose whether they want to do method chaining or just panic:

// Something just panics on error, but otherwise returns a Foo so method chaining works.
func (f Foo) Something() (Foo) {
    g, err := f.SomethingErr()
    if err != nil {
        log.Panic(err)
    }
    return g
}

// SomethingErr returns an error, for those callers that need to keep running after handling the error.
func (f Foo) SomethingErr() (Foo, error) {
    // actual implementation of Something goes here
}

There may be a more idiomatic way to do this, or at least better naming than the Err suffix I'm using above. Ideas welcome.

[deadsy]

bezier.go log.Panicf("can't place a handle on a curve midpoint")

That's the issue. Using the functions in a chained fashion is valuable/concise so with your proposal you are still going to get a panic in most (all?) usages. I'm fine with it staying as a direct panic. It's a non-optional signal that you need to fix the code.

[stevegt]

It's a non-optional signal that you need to fix the code.

I have use cases where method arguments, instead of being hardcoded, are calculated as part of a parameter study or genetic algorithm in a long-run process. In those cases, for non-viable individuals I produce null results and continue with the next parameter set in the list. With the genetic algorithm use case in particular it will be easy to generate panics that would interrupt a process that might have been running for days.

In the cold light of day it looks like a better solution, rather than the verbose mess I posted last night, would be for callers to just use Go's recover() to catch those panics. That would allow for method chaining and keep sdfx core simple. I'll try to get a chance to play with that idea today.

Assuming that works out, then the only thing in the way of closing this bug would be to go back and make sure we didn't break method chaining anywhere else. None jumped out at me during a quick egrep, but I think we'll have time over the next couple of days to review this during our internal calls.

[deadsy]

I buy the idea that you want the code to be tolerant of bizzaro values being passed as a parameters. But... the case we talking about is control points for bezier curves- you can't put a handle on a non-endpoint. Or at least, you could but it would be non-functional and the goal has been to resolve user misapprehension sooner rather than later.

sooner = at the call (panic) later = at the fixup call (error) latest = after a debugging session when they finally work out why it doesn't work

[stevegt]

We finally got a chance to spend the afternoon on a call going through the deltas since 0700ea8 -- we didn't find any other cases ourselves where method chaining got broken with all the recent error handling changes. The closest we found was Bezier.Polygon. It used to return Polygon, but now returns (Polygon, error) -- changed in parallel by both us and Jason. But it's a method on Bezier anyway, rather than Polygon, so not strictly method chaining in the first place.

Everything else we spotted were functions rather than methods. We had added err returns to these, which would prevent them from being used as the head of a chain:

ExtrudeRounded3D                                                     
Washer3D                                                      
Loft3D                                       
ThreeArcCam2D         

...and Jason added err returns to these functions the same way, some recently and some as far back as 2017, so we think we're on the same wavelength as far as his intent:

PanelBox3D                                           
TruncRectPyramid3D                                                    
MakeThreeArcCam                                                                                    
MakeGenevaCam                                                
MakeFlatFlankCam                   

So I think we may be good now. I'm ready to close this bug; I'll leave it for a day or few and will close it myself if @deadsy has no objections and doesn't close it first.

[stevegt]

I neglected to mention that we also spent some time a few days ago playing with Go's recover() mechanism. It looks like recover() will work well for those callers who need to handle panics and continue. This might best be put into a FAQ, but for now here's the google fodder for later travelers:

How to handle an sdfx panic

Use recover() to handle panics. This is a less-idiomatic way of doing things in Go, but it does enable the sdfx library to remain simple and support things like method chaining. I'm using try/catch vocabulary here, but you can of course name these functions whatever you want. There are other ways of doing this as well, such as deferring an anonymous function for the catch.

func catch() {
        errmsg = recover()
        // errmsg contains the panic() arg
}

// Before doing something that might panic, all we need to do is defer
// some sort of catch-like function that calls recover().
func try() {
        defer catch()
        SomethingThatMightPanic()
        fmt.Println("this line won't run if there was a panic")
}

func parent() {
        try()
        fmt.Println("execution continues here if there was a panic")
}

A more complete example is at https://play.golang.org/p/PMR0S30vkH4