Add children getter to entity #10324

jdfwarrior · 2022-04-26T02:21:48Z

Adds a children getter to the Entity prototype so that you can retrieve a list of children associated to the entity without having to access the private _children property of the entity.

It does not provide a setter since that is outside of the scope of my needs. I currently use the _children property but causes issues in TypeScript because I'm accessing a property that it believes doesn't exist on the object.

cesium-concierge · 2022-04-26T02:21:51Z

Thanks for the pull request @jdfwarrior!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.

ggetz · 2022-04-26T13:07:45Z

Thanks for the PR @jdfwarrior.

@mramato Is there a reason we don't currently expose children, even read-only, as a part of the public API?

mramato · 2022-04-26T13:12:58Z

From #2600

On an architectural note, Entities, which previously only knew about their parent, now also know about their children. This was needed to properly propagate setting a parent's show property without dealing with tons of notification subscriptions. I didn't expose this publicly yet because I want to make sure there aren't ramifications that I didn't think of

So there's no explicit reason other than "Are we sure this won't create problems or cases where it doesn't work as expected". It's been a while, so my memory is fuzzy but from what I remember the API itself isn't really set up for it, for example, what should happen if an entity has a parent and you push it onto the children array of another entity? Ideally the children array would be readonly, but that's probably non-obvious and an easy mistake for a dev to make.

If someone thinks through all of the edge cases and documents/handles them in a sensible way, I have no objections

jdfwarrior · 2022-04-26T13:17:41Z

@ggetz @mramato And so, ust for clarification, thats why a setter wasn't even looked at. I only made the getter. I don't want to be able to manipulate it. For the application that I currently work on, I have UI components that, when I click on an entity, I have my own custom info box that appears and one of the sections within it, lists the entities that are children of the current selected entity.

If this is approved, my next step would be to look into adding an event for when the childrens list is updated so that I have an event to trigger an update of the list.

ggetz · 2022-04-26T13:42:35Z

Source/DataSources/Entity.js

+  /**
+   * Gets the list of all children currently associated with this entity
+   * @memberof Entity.prototype
+   * @type {Entity[]}


Suggested change

* @type {Entity[]}

* @readonly

* @type {Entity[]}

This will mark the property explicitly as read only in the docs and the generated typescript definitions.

Also I believe what @mramato was alluding to is that while, yes, this is only a getter and not a setter, its possible to get the array of children and then mutate it by adding or removing elements. Either of which could have adverse effects.

We work around this in other classes such as ClippingPlaneCollection by not providing direct access to the internal array, and instead providing interfaces for getting the array length (length) and accessing each element by index (get(i)).

@ggetz So is there any point in adding the @readonly argument to the jsdoc block or should there just be a different implementation of this?

We could also spread or copy the original array to prevent it from being mutated as well. If that is considered a viable alternative, I can make that change. If so, seeing as how there are multiple methods of cloning an array, is there a preferred method?

So is there any point in adding the @readonly argument to the jsdoc block or should there just be a different implementation of this?

Not if this specific getter goes away, I just wanted to quickly point it out in case it comes up again.

We could also spread or copy the original array to prevent it from being mutated as well.

I see what you're saying, but I'd hesitate to recommend this way. I don't see anything similar in the rest of the codebase, most likely for performance reasons. It could get expensive if the getter is called frequently and if there are enough entities.

ggetz · 2022-05-05T12:53:48Z

Hi @jdfwarrior, do you think you'll get back to this shortly? If not, in order to keep things tidy, I would ask to close this until then and re-open when it's ready for another review.

jdfwarrior · 2022-05-06T03:00:49Z

@ggetz Yeah I'd love to get this done. I guess I just want to know what the implementation is that the team would actually want.

From what it seemed, the getter is acceptable except for the fact that, even though it's a getter, because it's an array it can still be mutated, which, is understandable. Is returning a copy of the array 100% off the table as an option? There are several options for doing this. You could spread or slice the original array to create a new one.

I can't unequivocally say that nobody would ever do it but, I don't see a case where anyone would ever want to call this repeatedly in a tight loop where performance would become an issue. It's a simple object but I created an object with several properties and filled an array with 100k of that object. Just using a basic console.time to check time on the spread and slice, both seemed to copy the array in 1-2ms over and over.

If returning a copy of the array is still considered not to be a good option and you'd prefer that I use a method to get children at a certain index, I can do that. Would there be a recommended function name? The other areas that I know of that functions like that exist are on objects that are collections, DataSourceCollection, EntityCollection, etc so a get or getIndex makes sense but when referencing a single entity and it's children, how would you prefer it be named? Something like getChildAt(index: number)? Or just getChild(index: number)? Just get seems too generic and wouldn't properly communicate what it does (personal opinion).

ggetz · 2022-05-06T13:15:40Z

If returning a copy of the array is still considered not to be a good option and you'd prefer that I use a method to get children at a certain index, I can do that.

Yes, I think we want to go that route for consistency with the rest of the API.

Would there be a recommended function name? Just get seems too generic and wouldn't properly communicate what it does

Good point. I think getChild(index: number) makes sense.

… method to get child at index

jdfwarrior · 2022-05-07T03:57:31Z

Ok. I removed the previous children getter that returned the array and replaced it with a getter that would return the count of children in the array and then added the getChild method that will return the child entity at the provided index and throw an error if undefined was passed. Literally copy/pasted the get method from DataSourceCollection to ensure that it followed the same style and method as other areas of the API. I kept the getter with the child count so that you can check that and hopefully know how many you're working with instead of having to run a loop until get undefined with getChild.

Does this seem like a better approach?

cesium-concierge · 2022-08-05T02:00:08Z

Thanks again for your contribution @jdfwarrior!