cesium Add children getter to entity

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.

Apr 26 '22 02:04 jdfwarrior

Thanks for the pull request @jdfwarrior!

:heavy_check_mark: Signed CLA found.
:grey_question: 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.

Apr 26 '22 02:04 cesium-concierge

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?

Apr 26 '22 13:04 ggetz

From https://github.com/CesiumGS/cesium/pull/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

Apr 26 '22 13:04 mramato

@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.

Apr 26 '22 13:04 jdfwarrior

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.

May 05 '22 12:05 ggetz

@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).

May 06 '22 03:05 jdfwarrior

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.

May 06 '22 13:05 ggetz

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?

May 07 '22 03:05 jdfwarrior

Thanks again for your contribution @jdfwarrior!