Bug calling two local functions in two foreach constructs in OnUpdate causes burst codegen errors

If you call two local functions in two Entities.Foreach constructs in a single OnUpdate then burst getscode gen issues. The case number is 1343411

Obviously this is nothing urgent to fix or ... and I've tried this with the burst compatible with latest entities and not sure if this is an Entities issue or burst issue but posted here in case it is one of those which is hard for QA to deal with.

 

Entities that does the codegen, not burst, so it probably belongs on the dots forum.

 

Well maybe it is in the code that foreach generates or maybe it is in the burst compilation, not sure which but probably you are right. will post there too.

 

It's known, Roslyn DisplayClass-es with Unitty modifications causes many errors with scopes\local functions.
(you can look at my latest explanation of an issue https://forum.unity.com/threads/usi...lently-get-wrong-values.1067135/#post-6892841)
Unity devs already working on a new implementation (one year already) and went out from DisplayClass-es.
I'm asking devs from time to time, and the last time was 4 month ago (March 2th), and Joel confirmed it's still the thing and they're actively working on it, no ETA. On our side, we can just wait.

 

It is not a big deal really! and refactoring a codegen as big as this takes time so I'm ok with smaller issues like this to remain until they make sure that the source generator based impl is robust and stable and I'm sure they had no reason to ship it alone as a change so it will ship with other big features/after them.

I just hope some good update comes at GDC for character animation

 

It's a big deal if you don't know the details of the problem and how to avoid it. I've had a couple of bugs that ended up tracing back to this issue, and debugging them was horrible because "maybe the compiler did the wrong thing" is not something that I'm used to considering in C#. It's manageable once you know about it, but I don't think that I've even seen it documented anywhere, so most people will be hit quite badly on their first encounter with it. If you don't know the details of how Roslyn's display classes work internally, you'll have to study quite a bit just to know how to avoid it.

It would help a lot if they could at least make it into a diagnostic error until the fix arrives instead of silently doing the wrong thing.

 

well yes in tht case. I only had it for local functions and thought it is triggered only by those. if that is the case and you test early and often it should not give you too much headache but it could be , I'm not denying that.

But it is preview tech, they way that eizenhorn was talking about it was sounding like a show stopper feature however.

 

I discussed the first time that I came across it in a thread here.

In that case, a local scope - which is usually innocuous - in the

Entities.ForEach

lambda was breaking the behaviour.

I had something like this:

Code (CSharp):

1. if (condition)
2. {
3.     // conditional body here
4. }

As I was fiddling with the code, I quickly commented out the condition like this:

Code (CSharp):

1. //if (condition)
2. {
3.     // conditional body here
4. }

And got nonsense behaviour. I spent ages trying to work out what incredibly stupid thing I was doing that would explain the behaviour I was seeing. It took me a long time to try removing the braces, as I'm not used to that being able to make any difference to behaviour - by spec, it can't. I looked at lots of other ways the compiler might be failing before I thought to try that. Lo and behold it fixed it.

In hindsight it's obvious and I could have gotten to the answer faster by being smarter about looking at the decompiled output early in the process, but it was a hair puller. I'm reasonably experienced with C# and have worked with Roslyn directly before and it still took me a bit, so I can imagine it may be even harder to catch for someone a little less experienced.

It's not mentioned in the Known Issues page for Burst or anything - it should at least be there! Pinging @tim_jones - could we at least get this mentioned in the documentation please? I know that it's an Entities issue not a Burst issue, but that page already discusses some things to do with

Entities.ForEach

so it fits there too.

 

Thanks for pushing this. I think that while there is a bug that can manifest for users, it should be documented somewhere visible, but obviously getting it fixed is the best option!