Defer property replay until objects materialize

[SeanCurtis-TRI]

Calls to set_property() and set_object() can be interleaved such that a set_property() is invoked on a path that hasn't completed its asynchronous load yet.

This queues up property invocations until the object is loaded and then replays them on the object.

---

This change is 

[SeanCurtis-TRI]

+a:@jwnimmer-tri for feature review, please.

[jwnimmer-tri]

@jwnimmer-tri reviewed 1 file and all commit messages, and made 4 comments.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on SeanCurtis-TRI).

---

a discussion (no related file):
The Readme.md says this, which is out of date now:

If a name in the property chain is missing, an error message will be printed to the console and no value will be assigned.

Similarly the chained_properties.html test case has this, which is out of date now:

        console.info("\nSetting a property that doesn't exist at all. We should " +
                     "get an error indicating that fact and the guidance that the " +
                     "property has not been set.");

---

src/index.js line 777 at r1 (raw file):

        // Store property for replay when object materializes (e.g., after async
        // glTF load)
        this.pending_properties[property] = {value, target_path};

It seems like we add the property to "pending" even if it was correctly set. Why is that? The purpose of pending seems to be to handle the case of objects not existing yet, we don't seem to be checking for that.

This seems to be doing the set_property operation twice, which both wastes time and makes it difficult to reason about sequencing in case the same property is being repeatedly changed.

---

src/index.js line 823 at r1 (raw file):

            // The child doesn't exist yet; we'll queue the property
            // assignment to handle asynchronous creation.
            this.pending_properties[property] = {value, target_path};

The property we are storing here has already been canonicalized (replacing [] with . and removing any leading .). However, up above during set_property we store the non-canonical property as the dictionary key.

In case canonicalization actually changed the string, that means there will be two different property values stored, with different-but-equivalent keys. If the user sets the same property via multiple different aliases that all canonicalize to the same name, then the "last one wins" invariant might get broken.

---

src/index.js line 860 at r1 (raw file):

        for (let property in queued_props) {
            let {value, target_path} = queued_props[property];
            this.set_property(property, value, target_path);

If the property was still unknown, I believe this.set_property will add it back into the queue, indefinitely. Shouldn't the bad set_property eventually time out with an error, so that (1) the user gets feedback and (2) we don't leak memory (and time spent retrying) indefinitely?

[SeanCurtis-TRI]

@SeanCurtis-TRI made 4 comments.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on jwnimmer-tri).

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The Readme.md says this, which is out of date now:

If a name in the property chain is missing, an error message will be printed to the console and no value will be assigned.

Similarly the chained_properties.html test case has this, which is out of date now:

        console.info("\nSetting a property that doesn't exist at all. We should " +
                     "get an error indicating that fact and the guidance that the " +
                     "property has not been set.");

Well spotted. I actually overhauled the relationship between property chains and property replay in order to keep the error conditions clean.

---

src/index.js line 777 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It seems like we add the property to "pending" even if it was correctly set. Why is that? The purpose of pending seems to be to handle the case of objects not existing yet, we don't seem to be checking for that.

This seems to be doing the set_property operation twice, which both wastes time and makes it difficult to reason about sequencing in case the same property is being repeatedly changed.

As part of the effort to better coordinate with chained properties, this has been clarified, I hope.

---

src/index.js line 823 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The property we are storing here has already been canonicalized (replacing [] with . and removing any leading .). However, up above during set_property we store the non-canonical property as the dictionary key.

In case canonicalization actually changed the string, that means there will be two different property values stored, with different-but-equivalent keys. If the user sets the same property via multiple different aliases that all canonicalize to the same name, then the "last one wins" invariant might get broken.

This has now changed. PTAL.

---

src/index.js line 860 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If the property was still unknown, I believe this.set_property will add it back into the queue, indefinitely. Shouldn't the bad set_property eventually time out with an error, so that (1) the user gets feedback and (2) we don't leak memory (and time spent retrying) indefinitely?

I believe that we're ok in the new infrastructure. PTAL.

[jwnimmer-tri]

@jwnimmer-tri reviewed 1 file, made 4 comments, and resolved 2 discussions.
Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on SeanCurtis-TRI).

---

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Well spotted. I actually overhauled the relationship between property chains and property replay in order to keep the error conditions clean.

The new complexity in the Readme is a sign that we've fallen down a complexity hole. See below for simplification.

---

src/index.js line 777 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

As part of the effort to better coordinate with chained properties, this has been clarified, I hope.

I think this can all be vastly simpler and more precise -- zero heuristics -- if I've understood correctly.

(1) Don't infer is_object_placeholder. Instead, have passed in via a new constructor argument (defaults to false). Maybe rename it to "loading_in_progress" or something.

(2) In set_object_from_json, don't wait to call find until after the mesh has loaded. Call it right away, so that all of the SceneNodes are immediately created. For its final <object> node, set loading_in_progress to true.

(2b) If the final node already existed (i.e., if we are replacing it), we probably also need to clear out the old geometry immediately (so that properties won't be set on the stale geometry). Depends on exactly how the logic ends up being written.

Now we know exactly which scene nodes are pending, so we can precisely either apply a property, defer it until after the loader finishes, or report an error immediately. It remains always an error for a set_property to refer to a scene node that doesn't exist.

(3) In SceneNode.set_property, we should enqueue the property for later application (during set_object) exactly when either

(3a) it's being set on a node where loading_in_progress is true, or

(3b) for recursive properties (materials) only, on any descendant nodes that are still marked with loading_in_progress. It must not be enqueued on their parents, only on the specific in-progress <object> leaves.

(4) If you like, one cute representation trick would be to have pending_properties !== undefined be the representation of the loading_in_progress bool. In other words, if the SceneNode should accept pending properties, then it has a dict to store them. If not, then the attribute is undefined. No need to keep two member fields in sync.

---

src/index.js line 826 at r2 (raw file):

    }

    set_property_chain(property, value, target_path, defer_if_missing = false) {

It doesn't seem to me like defer_if_missing is a good name here. When missing, nothing in this function defers anything. (To me, "defer" means "schedule for later", not "ignore".) In terms of this function, this flag only chooses whether errors are reported vs ignored.

---

src/index.js line 874 at r2 (raw file):

        }

        if (error_detail != null) {

nit Wouldn't it be simpler to handle the "ignore errors instead of reporting errors" right here inside this if, without any of the other changes in this function? This is the single point of control for "report and error" so seems to me like a more obvious place to nerf it.

[SeanCurtis-TRI]

@SeanCurtis-TRI made 4 comments and resolved 2 discussions.
Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on jwnimmer-tri).

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The new complexity in the Readme is a sign that we've fallen down a complexity hole. See below for simplification.

Or it could be a measure of previous imprecision that has been made more precise while adding a new feature. I don't see the complexity that you see.

---

src/index.js line 777 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think this can all be vastly simpler and more precise -- zero heuristics -- if I've understood correctly.

(1) Don't infer is_object_placeholder. Instead, have passed in via a new constructor argument (defaults to false). Maybe rename it to "loading_in_progress" or something.

(2) In set_object_from_json, don't wait to call find until after the mesh has loaded. Call it right away, so that all of the SceneNodes are immediately created. For its final <object> node, set loading_in_progress to true.

(2b) If the final node already existed (i.e., if we are replacing it), we probably also need to clear out the old geometry immediately (so that properties won't be set on the stale geometry). Depends on exactly how the logic ends up being written.

Now we know exactly which scene nodes are pending, so we can precisely either apply a property, defer it until after the loader finishes, or report an error immediately. It remains always an error for a set_property to refer to a scene node that doesn't exist.

(3) In SceneNode.set_property, we should enqueue the property for later application (during set_object) exactly when either

(3a) it's being set on a node where loading_in_progress is true, or

(3b) for recursive properties (materials) only, on any descendant nodes that are still marked with loading_in_progress. It must not be enqueued on their parents, only on the specific in-progress <object> leaves.

(4) If you like, one cute representation trick would be to have pending_properties !== undefined be the representation of the loading_in_progress bool. In other words, if the SceneNode should accept pending properties, then it has a dict to store them. If not, then the attribute is undefined. No need to keep two member fields in sync.

That's a really dense list. Much of it is statement of solution with which I'm struggling to understand the problems being solved.

As I worked my way through your suggestions (responses, such as they are, below), one theme kept coming to mind. There seems to be an assumption that one call to set_object creates one <object> instance (as child of the named path). That is not generally the case.

Generally, I think this needs more discussion.

---

(1) I don't know what you mean by "don't infer". It's not inferred, it's purely the context. When following a node path creates a new child, it automatically sets that to be a place holder. This is the one time that the introduction of a SceneNode can be known to be a place holder. Because it was created via traversal and not assignment. The only thing that clears it is actually setting the object (via SceneObject.set_object()). That doesn't seem like a heuristic. So, the only difference I can see that your comment implies is that in create_child() where we set is_object_placeholder we would, instead, pass the flag to add_child that would in turn pass it to the SceneNode constructor. Is that less complex?

(2a) I'm perhaps being a bit obtuse about set_object_from_json. "For its final <object> node, set loading_in_progress to true." If the object_json is a glTF file, it won't necessarily be a final <object> I could be an entire tree. I'll have to think about the implications of what you're saying vis a vis that fact.

(2b) Again, "we know exactly which scene nodes are pending" is not generally true.

(3) Again, handling set_object is not necessarily a single object.

---

src/index.js line 826 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It doesn't seem to me like defer_if_missing is a good name here. When missing, nothing in this function defers anything. (To me, "defer" means "schedule for later", not "ignore".) In terms of this function, this flag only chooses whether errors are reported vs ignored.

Done

---

src/index.js line 874 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Wouldn't it be simpler to handle the "ignore errors instead of reporting errors" right here inside this if, without any of the other changes in this function? This is the single point of control for "report and error" so seems to me like a more obvious place to nerf it.

Agreed.

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on SeanCurtis-TRI).

---

src/index.js line 777 at r1 (raw file):
Yes, I posted a solution instead of trying to spend time itemizing all of the ways in which the current should_queue_property_for_replay heuristic fails. If it helps, we can do a call next week to go over them.

When following a node path creates a new child, it automatically sets that to be a place holder.

Yes, it's marked as a placeholder if it's new and it's named <object>. However, not all <objects> are destined to be replaced by a set_object call. Maybe the user just decided to name something that. We have no limitation that <object> as an element in the path is reserved for use by set_object.

The heuristic is that <object> is only ever emplaced by set_object tacking it onto the end of the path. That's not an invariant, it's just a guess.

If the object_json is a glTF file, it won't necessarily be a final <object> I could be an entire tree. I'll have to think about the implications of what you're saying vis a vis that fact.

Good point, I didn't realize that. In that case my proposal extends to the glTFs root <object> being marked as "loading in progress", and then any deferred properties can be applied recursively once it appears.

The key invariant is that set_object_from_json must mark something specific as "loading in progress" and that properties must only be deferred for something specifically marked as such. Trying to guess which properties couldn't be applied probably-because they are still being loaded is too difficult to make accurate.

My original proposal only marked leaves as "loading in progress", but with the "loading can add whole subtrees" we need to amend it to allow marking subtress as well. But the point is that it must be marked as "this thing is pending because I really do have an async load call targeting it". Using properties of the path to guess whether something is pending is the main defect.

[SeanCurtis-TRI]

src/index.js line 777 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes, I posted a solution instead of trying to spend time itemizing all of the ways in which the current should_queue_property_for_replay heuristic fails. If it helps, we can do a call next week to go over them.

When following a node path creates a new child, it automatically sets that to be a place holder.

Yes, it's marked as a placeholder if it's new and it's named <object>. However, not all <objects> are destined to be replaced by a set_object call. Maybe the user just decided to name something that. We have no limitation that <object> as an element in the path is reserved for use by set_object.

The heuristic is that <object> is only ever emplaced by set_object tacking it onto the end of the path. That's not an invariant, it's just a guess.

If the object_json is a glTF file, it won't necessarily be a final <object> I could be an entire tree. I'll have to think about the implications of what you're saying vis a vis that fact.

Good point, I didn't realize that. In that case my proposal extends to the glTFs root <object> being marked as "loading in progress", and then any deferred properties can be applied recursively once it appears.

The key invariant is that set_object_from_json must mark something specific as "loading in progress" and that properties must only be deferred for something specifically marked as such. Trying to guess which properties couldn't be applied probably-because they are still being loaded is too difficult to make accurate.

My original proposal only marked leaves as "loading in progress", but with the "loading can add whole subtrees" we need to amend it to allow marking subtress as well. But the point is that it must be marked as "this thing is pending because I really do have an async load call targeting it". Using properties of the path to guess whether something is pending is the main defect.

Ah...this time you're referring to should_queue_property_for_replay (I think you may have had a typo that led me down the wrong path). That, I agree, is a heuristic. So, all of your proposals are towards making that heuristic disappear in favor of a unambiguous signal. Let me ruminate on that in conjunction of all the other stuff.

[SeanCurtis-TRI]

src/index.js line 777 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ah...this time you're referring to should_queue_property_for_replay (I think you may have had a typo that led me down the wrong path). That, I agree, is a heuristic. So, all of your proposals are towards making that heuristic disappear in favor of a unambiguous signal. Let me ruminate on that in conjunction of all the other stuff.

(BTW I wouldn't happily accepted one case in which the heuristic fails....I didn't need an itemized list.)