BUG: NVISII-125 Missing check on lazy_updates in initializeHeadless

[Neburski]

The initializeHeadless function did not contain the logic to handle the option where lazy_updates was set to false. This leads to incorrect data for the first frames.

[natevm]

I understand what you’re trying to fix, but this change doesn’t really make sense. Headless mode always has lazy updates enabled. Lazy updates are for interactive mode only, to try to make interactive mode have the same performance as headless mode. As it stands, these changes introduce a performance regression for headless mode by unnecessarily rebuilding / recompiling scene updates.

This also is not a fix for the bug you reference, but rather a workaround. It’ll be harder to do, but we need to figure out what’s causing the race condition in the bug you referenced.

[natevm]

Closing for now, but feel free to comment if you feel different.

[Neburski]

It is up to you to decide if this is merged or not. There are no hard feelings on my side for your decision against it.

From my point of view: This change doesn't change the behavior of NVISII if lazy_updates = True is used at the initialize step because the 'core' of my change wouldn't be executed, thus falling back to the original behavior. It could however introduce a performance regression for existing scripts which set headless = True, but which don't specify lazy_update = True. This certainly could be a concern for some.

This merely offers what looks like a relatively clean solution to the issue. After all, there are four potential cases when using two boolean flags. Up to now, only three out of the four cases were used, this adds the fourth case.

It was my impression that the initialize function would only be called once, thus I wasn't expecting a potential performance regression for the headless case.

we need to figure out what’s causing the race condition in the bug you referenced.

Certainly, if there is a race condition somewhere else, then it would be good if that is resolved properly. However, that is somewhat out of my 'skill set' and knowledge of the code base ;-)

p.s.: I'm quite grateful for your work on this project and the quick response time. Apologies for me barging in like a beginner and firing off issues and pull requests.

[natevm]

No worries, I appreciate the participation! Please keep “pestering” us, haha.

Yeah, I see now where you are coming from. In retrospect, my time is a bit limited, so a workaround might be warranted until I have time to dig into the race condition. Race conditions are pretty hard to debug, so I’d probably need to be the one to do it.

The issue with the workaround is mostly this perf regression in headless mode. Honestly, I’d prefer we don’t have this lazy_updates flag at all. That flag itself was a bit of a workaround to get interactive mode scripts to run a bit quicker. It’s not super clear to end users what that lazy updates flag does.

I think I’m still leaning towards not merging this pull request, but it’s interesting to know that forcing lazy updates to on fixes these race conditions.

[Neburski]

it’s interesting to know that forcing lazy updates to on fixes these race conditions

To my understanding, it should be when setting lazy updates to off, note the ! in the if condition. That might be important to keep in mind when you get around to debugging this.

I noticed that the header file include/nvisii/nvisii.h has different argument names compared to the src/nvisii/nvisii.cpp. I'm assuming that is intended. I never knew that was possible (I'm not a programmer by education).

[natevm]

Yes, the header parameter name differences are intentional. In C++, the declaration and definition only need to match in type sequence, and the parameter names can be different between the declaration and definition. We abuse this so that Python key word arguments follow snake_case rather than lowerCamelCase.