-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Serialize lights into gfx-replay json format. #1857
Conversation
9f1befe
to
96431df
Compare
1f571f1
to
8a65ae5
Compare
8a65ae5
to
90dea40
Compare
90dea40
to
6acb9bb
Compare
8380fea
to
5933fbc
Compare
5ccb0bf
to
07694ba
Compare
07694ba
to
a75daa4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far. I left a few comments.
src/esp/gfx/replay/Keyframe.h
Outdated
@@ -55,6 +55,7 @@ struct Keyframe { | |||
std::vector<std::pair<RenderAssetInstanceKey, RenderAssetInstanceState>> | |||
stateUpdates; | |||
std::unordered_map<std::string, Transform> userTransforms; | |||
Cr::Containers::Optional<std::vector<LightInfo>> lights; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little torn on serializing our entire quirky LightInfo
struct (including LightPositionModel). I talked in the design doc about wanting gfx-replay to be agnostic to the renderer, and most renderers IMO won't offer "object-local" or "camera-local" lights. I think we only need to support lights specified in global space. Also, I don't think anybody is using object-local or camera-local lights in Habitat. I don't think we'll bother implementing them in the new batch renderer.
See also this discussion here: https://cvmlp.slack.com/archives/G0131KVLBLL/p1660649432149979 .
It is probably fine to leave this as-is, but let's beware introducing unneeded complexity into the gfx-replay format in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree.
I feel like it doesn't hurt to leave the LightPositionModel
field as-is for now. We should look into specifying a standard light data layout as we integrate them with the batch renderer.
I am still unfamiliar with the batch renderer, but if we are following a pure ECS approach, we might want to separate the transform component from the light data.
I think we only need to support lights specified in global space.
Object-local lights may be occasionally useful (e.g. if the simulated agent has a light). However, I don't think that we are doing anything like that at the moment.
I don't know how camera-local lights would be useful.
src/esp/io/JsonMagnumTypes.h
Outdated
Magnum::Vector3 vec3; | ||
for (rapidjson::SizeType i = 0; i < 3; ++i) { | ||
if (obj[i].IsNumber()) { | ||
vec3[i] = obj[i].GetDouble(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vector3 stores floats right? Why do we keep calling getDouble? Are we also serializing it as a double? These storing floats as doubles add up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON's only numeric type is doubles, yes. Not sure if rapidjson even has 32-bit-float APIs, usually there isn't any significant difference in parsing as a float or a double.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh darn, didn't know that about JSON. Yet another reason to avoid using it for our dataset.jsons in Habitat-Lab. 0-0 You can set the max decimal places in RapidJSON globally it appears, but other than that no. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrade's Utility::Json has an option to switch between floats / doubles (and 32/64bit integer types), but not even there it makes any difference memory-usage-wise (it abuses the IEEE floating-point layout to store integers inside a double representation, and in all cases it's stored directly in parsed token memory that just has to be of certain size), and until I have my own optimized floating-point parser that has a dedicated code path for 32bit floats, it won't make any difference for parsing speed either. It's there purely for convenience.
But limiting decimal places (or excessive line wraps / indentation for arrays) could make a difference in subsequent parsing time, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: see usePrettyWriter
and maxDecimalPlaces
in writeJsonToFile
. I added this in a half-hearted attempt to minimize the filesize of gfx-replay files.
…of empty vectors." This reverts commit e85d8ac.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Motivation and Context
This changeset adds light serialization to the gfx-replay format.
Lights are serialized incrementally - they are only written to the json file when lighting changes occur. We assume that the keyframes are always read sequentially.
`GfxReplayTest::testLightIntegration` Replay File
How Has This Been Tested
GfxReplayTest::testLightIntegration
).Types of changes
Checklist