Hi, I’m seeing issues where FNiagaraSystemCompilingManager::GetNumRemainingAssets() returns a result of 0, but if any other IAssetCompilingManager returns >0 then FNiagaraSystemCompilingManager::FinishAllCompilation() actually does trigger compilation of Niagara Systems. This seems to be because one or more Niagara System will end up returning true from HasOutstandingCompilationRequests(), but that is not checked by GetNumRemainingAssets(). I’m not sure if the expectation is that if HasOutstandingCompilationRequests() returns true, then the compiling manager should have a QueuedRequest or ActiveTask reflecting that, or whether the issue is that GetNumRemainingAssets() needs to loop over the Niagara Systems checking for any that have pending requests.
The result of this is that I’m seeing inconsistent results with when Niagara Systems actually get compiled, which can lead to compilation errors being surfaced at unexpected times. For example, I’m trying to modify our automated test run commandline to wait for all asset compilations to complete before running tests to prevent async compilations triggered by startup loads (e.g. due to Gameplay Cue Notifies loaded on startup) from failing whatever test happens to be running when the compilations complete. I can’t get this to work consistently due to FX reporting no pending work to do, and yet it does have pending work that ends up being inconsistently triggered later on due whenever other IAssetCompilingManagers trigger it all managers to finish their work.
As a proof-of-concept I added the loop over Niagara Systems in FinishAllCompilation() to GetNumRemainingAssets(), and that allows things to work consistently. However I’m not familiar enough with the Niagara Compiling Manager or Niagara Systems to know whether this is an appropriate change or whether there’s a higher level issue with the Niagara Systems’ pending requests not being flushed into QueuedRequests in the compiling manager.
Steps to Reproduce
One repro is that we have Gameplay Cue Notifies that reference some Niagara Systems, causing the Niagara Systems to be loaded on startup and then request pending an async compile. Those async compiles don’t get processed until or unless some other compiling manager triggers a compilation or something calls FinishAllCompilation() directly.
I’m also seeing an issue where launching Unreal Editor, selecting a Niagara System in the Content Browser, and running data validation on it does not cause the system to be compiled even though the EditorValidatorSubsystem explicitly calls FAssetCompilingManager::GetNumRemainingAssets() and then conditionally FinishAllCompilation().
The intent behind the FNiagaraSystemCompilingManager is to manage all active compilations rather than managing the systems themselves. So, it’s interpretation of GetNumRemainingAssets() does seem like it’s at least representative of that intent…but I can understand that that’s maybe not a very helpful answer.
It might be interesting to see if things behave a bit more like you’d expect if you were to set fx.Niagara.OnDemandCompileEnabled to 0. This will make it so that the request to compile a dirty system is made on load rather than when a component using that system is activated.
We currently defer compilation where possible because of the non-zero cost (that we are continually trying to reduce!) of the compilation and there are many projects that reference many systems without actually using them.
I’ll see if we can try out modifying the test commandline and data validation to set OnDemandCompileEnabled to 0 so that they can start up & run more reliably.
And on a related note for data validation, we found that Niagara compilation errors were being logged as warnings, and thus not failing data validation like we would have expected. I traced this down to UNiagaraScript::SetVMCompilationResults explicitly logging the errors as warnings to avoid blocking cooks. Is there any way that code to only log the errors as warnings during the cook commandlet, but still log full errors in other contexts? This led us to realizing that we had a fair number of broken FX assets that no one had noticed until the compilation warnings happened to start breaking automated tests that were not expecting the async FX compilation errors-as-warnings logs (this is what led to my original post).
I don’t think we’d ever make it the default to generate an error, but I think it does make sense to mirror what the Material system does which has a cvar which controls whether errors should be logged (with UE_ASSET_LOG) as errors or warnings. r.MaterialLogErrorOnFailure…I’ll look to get that in here shortly.