Gfx-replay polish: workaround for material-overrides; new keyframe getter

[eundersander]

Motivation and Context

Some polish needed to get the batch renderer working in a new Python application.

How Has This Been Tested

Ran it locally.

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have completed my CLA (see CONTRIBUTING)
• I have added tests to cover my changes.
• All new and existing tests passed.

[0mdc]

LGTM!

[mosra]

Boolean arguments are an API smell -- writeSavedKeyframesToStringArray(false) could imply "not write", "not saved", "not array" or any other meaning until you actually check the API documentation.

For consistency with writeSavedKeyframesToString() i think the easiest would be adding a new writeIncrementalSavedKeyframesToStringArray() variant that doesn't call the consolidate*() inside.

[mosra]

Hmm, this just piles on more and more workarounds that were initially caused by the parser expecting a single keyframe to not be a {...} but rather {"keyframe": {...}}, which then means one then has to process [{...}, {...}, {...}] into [{"keyframe": {...}}, {"keyframe": {...}}, {"keyframe": {...}}] and it's all a lot more code and work and runtime overhead than if the API would accept just the {...} directly.

I remember already partially working around that for the replayer utility with adding a new variant that doesn't require the "keyframe" around -- could we switch to using that API always everywhere and reduce the two writeSavedKeyframesToString*() to be just one, returning either a [{...}, {...}] or a list of {...}, {...}? The former would be a lot less allocations, so preferrable from my side.

I assume the main reason for this vector-returning API to exist is to avoid a JSON parsing-splitting-serizalization roundtrip on the Python side, right? Separating a [{...}, {...}] to individual keyframes as string sub-ranges is doable in an efficient way with no string copies or number parsing for example like in the replayer utility below, so the Python bindings could be doing that instead.

habitat-sim/src/utils/replayer/replayer.cpp

Lines 112 to 126 in e6e3cfd

	/* Just tokenize the file and process the bare minimum needed to be able to
	go through the keyframe list */
	Cr::Containers::Optional<Cr::Utility::Json> jsonFile =
	Cr::Utility::Json::fromFile(filename);
	Cr::Containers::Optional<Cr::Utility::JsonObjectView> root;
	if (!jsonFile || !(root = jsonFile->parseObject(jsonFile->root())))
	Mn::Fatal{} << "Can't parse" << filename;
	const Cr::Utility::JsonToken* jsonKeyframes;
	if (!(jsonKeyframes = (*root).find("keyframes")) ||
	!jsonFile->parseArray(*jsonKeyframes))
	Mn::Fatal{} << "No keyframes array in" << filename;
	/* Put raw keyframe data into the array */
	for (const Cr::Utility::JsonToken& keyframe : jsonKeyframes->asArray())
	arrayAppend(keyframes_, keyframe.data());

[eundersander]

I agree with your suggestions but I'm just being lazy here. I think of all these keyframe APIs as temporary until we do a perf optimization pass and refactor them. We've talked about moving away from JSON altogether.

[mosra]

Right, then it doesn't matter that much. I wasn't aware that this was all meant to be temporary.

[mosra]

Suggested change

	"addFile failed for " << creation.filepath);
	"addFile failed for" << creation.filepath);