DynamicScene ES5 properties #1132
Conversation
1. Refactor DynamicBillboard to use ES5 properties and raise property changed events. 2. Refactor DynamicObjectCollection to improve the API and event notification. CompositeDynamicObjectCollection is busted until it is also refactored.
This also involves refactoring DynamicObject to generate proeprty notifications.
Get all tests to pass.
This is a huge performance regression until visualizers are optimized.
…hey are refactored. This fixes the performance regression created with last submit.
Remove DynamicObject.subPropertyChanged since it's only used by CompositeDynamicObjectCollection, which will get it's own version. Some specs failing because of the way we are exposing collections.
Rather than expose a collection property, wrap it so that we can always know when something has changed.
1. Added clone to all DynamicXXX objects because the composite needed it. 2. Add tests for both coarse and fine property compositing. 3. Rename collection methods to remove ambiguity. Everything should work but we still need tests for the collection level stuff.
Fix DynamicEllipsoid.clone which the test showed did not work.
Also take a first change at updating CHANGES.
| var id = dynamicObject.id; | ||
| var hash = this._hash; | ||
| if (defined(hash[id])) { | ||
| throw new DeveloperError('An object with id ' + id + ' already exists in this collection.'); |
There was a problem hiding this comment.
I think this is a RuntimeError, not a DeveloperError.
There was a problem hiding this comment.
I guess I see your point. At some point we need to do a thorough review of all of our exceptions and come up with some rules for when to throw them.
There was a problem hiding this comment.
My rough rule of thumb: DeveloperError means you coded it wrong, RuntimeError means the data is wrong, and also sort of indicates that you as the developer should be validating the data somehow.
Conflicts: CHANGES.md
…tCollection instances Rather than storing a property on the objects themselves, generate a unique key based on the object id, the id of the collection it is in, and the property name.0 Add an `id` property to both DynamicObjectCollection and CompositeDynamicObjectCollection; which is a readonly GUID generated at construction time.
|
@shunter This is ready for another look. |
Conflicts: CHANGES.md
|
@shunter I wasn't sure if there was anything else you needed from me. I'm pretty sure all of your comments have been addressed. |
|
I pushed the spec I showed you that fails because property change listeners clobber object change listeners in certain cases. I suggest changing your keys to something like |
|
Thanks @shunter good catch. This is now fixed and ready to go. |
|
Good to go. |
…properties DynamicScene ES5 properties
This change updates all of the DynamicScene graphics objects (
DynamicBillboardet al.) to use ES5 properties. It adds the ability to observe each of these objects via thepropertyChangedevent.CompositeDynamicObjectCollectionhas been rewritten to actually work in all cases and uses the new features to make that happen.DynamicObjectCollectionhas also had some API changes to improve usability. Both collections now offer additional notification information for when objects change. I'm not 100% happy with theCompositeDynamicObjectCollectionAPI, so I'm open for suggestions for additional tweaks.This change also adds the ability to easily specify custom properties on
DynamicObjectvia theaddPropertymethod. This allows for any data source to add custom properties which will then play nice with the compositing and notifications system. This is a precursor to automagically handling custom properties in CZML in a future pull request.Since ES5 properties are slow, the visualizers currently index into the "private" variables that back the properties. Future pull requests will re-write each of the visualizers in order to take advantage of the new functionality and improve overall performance.