Stop currentWorld check in decorateChunk

[Zeno410]

In the decorateChunk method of BiomeDecorator, there's this little check.

 

        if (this.currentWorld != null)
        {
            throw new RuntimeException("Already decorating!!");
        }

 

This check isn't usually correct, but it causes a lot of trouble in complex world generation code. It's very natural for decoration of one chunk to set off decoration of another chunk, and this goes off even though everything is working just fine. The variable should really be passed to the method rather than being an instanceVar, and maybe that's why this check is in (to deal with re-entrant code).

 

I can manage this in my own code, but I have chronic problems with bugs from other mods which aren't really bugs because if this code wasn't there everything would work. For a long time my solution was simply to erase the currentWorld variable of ALL the biome decorators whenever I ran my procedure, which runs in a post-decoration event, after all the uses of currentWorld are done. This worked nearly perfectly, just to indicate how useless that check is. But, now I'm doing some things in ore generation, and erasing the variables isn't an option, because now some things are using it.

 

So, can Forge code edit out this check? If there's enough of a need for a check against improper re-entrance into the generate chunk routine, then maybe add in a check that's correct.

[LexManos]

No, this is recursion protection, it is there for a very good reason.

And you're saying.. that when you use a specific subset of events that force the state into a small structured method you don't get errors... And when you expand to a larger decoration system you get errors... you dont say...

 

Basically, the point is, if you decorate the same chunk twice you're doing something wrong.

 

No, we're not going to remove this. I know for a fact that it has actually prevented quite a few mods from making it to the market that would break everything.

[Zeno410]

The problem is that it's not protecting against recursion, it's "protecting" against decorating two *different* chunks at the same time. Sure, that occasionally catches a recursion, but from managing all the bug reports that came up, it's less than 10% of the time. Plus, if you get a problematic recursion, it leads to infinite recursion and you get a stack overflow, so it's caught anyway. Recursion without infinite recursion doesn't cause problems - I expect performance is an issue but the chunks come out right, for my mods anyway.

 

If you really want to protect against recursion, you need a completely different procedure where you keep a HashSet of chunks being processed and skip calls to redo ones that are being done (or something similar). I actually ended up doing this, to catch the genuine infinite recursions that came up.

 

I don't think the current check is catching much actually, because any mod with complicated decoration is basically forced to shut the check down. As I said, I was clearing the vars. Highlands has apparently wrapped its calls in a try-catch because I see the stack trace for the call in my console logs. I suspect a lot of other mods are also try-catching their calls but not logging the stack traces.