Skip to content
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.

Sign up for GitHub

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

Add children getter to entity #10324

Closed
wants to merge 13 commits into from
Closed

Add children getter to entity #10324

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7df4595
feat: add children getter to entity
jdfwarrior Apr 26, 2022
8066c7f
feat: add children getter to entity
jdfwarrior Apr 26, 2022
641abb0
add name to contributors
jdfwarrior Apr 26, 2022
da12acd
add unit tests for new code
jdfwarrior Apr 26, 2022
79276cf
add comment to the changelog
jdfwarrior Apr 26, 2022
dbec768
Merge branch 'CesiumGS:main' into main
jdfwarrior Apr 26, 2022
e1522f5
Merge remote-tracking branch 'origin' into add-children-getter
Apr 26, 2022
635398e
Merge branch 'CesiumGS:main' into main
jdfwarrior May 7, 2022
b284e54
Merge branch 'main' into add-children-getter
jdfwarrior May 7, 2022
e4f50bb
chore: remove previous edits that added children getter
jdfwarrior May 7, 2022
da6b1d1
chore: added basic tests for new code added
jdfwarrior May 7, 2022
6e98272
feat: added children getter to return the children count and getChild…
jdfwarrior May 7, 2022
d8789d4
chore: update changelog
jdfwarrior May 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- Added support for morph targets in `ModelExperimental`. [#10271](https://github.com/CesiumGS/cesium/pull/10271)
- Added support for skins in `ModelExperimental`. [#10282](https://github.com/CesiumGS/cesium/pull/10282)
- Improved rendering of ground and sky atmosphere. [#10063](https://github.com/CesiumGS/cesium/pull/10063)
- Added `Entity.children` getter [#10324](https://github.com/CesiumGS/cesium/pull/10324)

##### Fixes :wrench:

Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,3 +317,4 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
- [Shen WeiQun](https://github.com/ShenWeiQun)
- [四季留歌](https://github.com/onsummer)
- [Yuri Chen](https://github.com/yurichen17)
- [David Ferguson](https://github.com/jdfwarrior)
10 changes: 10 additions & 0 deletions Source/DataSources/Entity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,16 @@ Object.defineProperties(Entity.prototype, {
this._definitionChanged.raiseEvent(this, "parent", value, oldValue);
},
},
/**
* Gets the list of all children currently associated with this entity
* @memberof Entity.prototype
* @type {Entity[]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @type {Entity[]}
* @readonly
* @type {Entity[]}

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
children: {
get: function () {
return this._children;
},
},
/**
* Gets the names of all properties registered on this instance.
* @memberof Entity.prototype
Expand Down
2 changes: 2 additions & 0 deletions Specs/DataSources/EntitySpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ describe("DataSources/Entity", function () {
expect(entity.show).toBe(options.show);
expect(entity.availability).toBe(options.availability);
expect(entity.parent).toBe(options.parent);
expect(entity.children).toBeInstanceOf(Array);
expect(entity.children.length).toBe(0);
expect(entity.customProperty).toBe(options.customProperty);

expect(entity.billboard).toBeInstanceOf(BillboardGraphics);
Expand Down