Some progress on the planner

Dirk Hohndel dirk at hohndel.org
Thu Apr 17 07:33:31 PDT 2014


Hi Robert,

thanks for picking up this 10 ton bolder and rolling it up the hill...

On Thu, 2014-04-17 at 10:54 +0200, Robert Helling wrote:
> the last days, I have spend some time with the planner. All this has
> happened on my “oneplannertable” branch on github but here I send the
> currents state of affairs as a single patch to me merged with master
> before Tomaz in his aim to separate out things touches every single
> line in every single file in his attempt to weed out legacy connections
> between C and C++ code.

:-)

I would have /preferred/ something half way in between what you have in
that branch and what you sent. But since it is all just redoing the
planner (which isn't on / used by default), I'll take the single patch
version.

What would have been nice (and could relatively reasonably have been
created from the branch) would have been a series of three or four
patches that separates out the major parts of this work, but maybe I'll
get that next time. I certainly am not going to send you back to
refactor the patches - that's just silly.

A couple of random comments:

- the ratio of comments to code in Subsurface has always been rather
low, to the point of making it often brutally hard to understand the
existing code. Especially with fragile things like the "finding the next
stop" algorithm I'd really love to see more comments that explain what
we are doing (and where appropriate - why).

- have you considered improving the debuggability of the code? And
adding sanity checks / asserts that verify that our implicit assumptions
are correct (oh, and add comments about those assumptions)?

> I guess I made some progress but the planner is still far from being
> complete or even presentable to the average user. Here is a list of
> things I did, Dirk, you may want to take the following as a commit
> message:

That's what I did.

> [Short rant break: Treating 0/0 as air bites back at so many places.
> E.g. Cylinder data is initialized with memsetting the whole structures
> to 0. Then later suddenly this totally unconfigured cylinder is being
> treated as it would contain air. Maybe at some point this was a
> feature. But it lead to a naughty bug which took me over an hour to
> resolve. We should seriously reconsider this choice and better move to
> 209/0 being air if changing this everywhere is not too much trouble]

I completely agree. Linus has been adamant from the beginning that this
was the way to do things and I am generally hesitant to simply ignore
his advice - but every time I touch any code that involves our gasmix
data structure I curse his conviction that this is the way to go.
The way things are today, an uninitialized / unused tank and a tank of
unknown size with air in it are indistinguishable.

/D



More information about the subsurface mailing list