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.

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

Display.sizeProperty does not use useDeepEquality #1152

Open
zepumph opened this issue Feb 10, 2021 · 2 comments
Open

Display.sizeProperty does not use useDeepEquality #1152

zepumph opened this issue Feb 10, 2021 · 2 comments
Labels

Comments

@zepumph
Copy link
Member

zepumph commented Feb 10, 2021

Here is what the TinyProperty callsite currently looks like:

    // @public {Property.<Dimension2>} The (integral, > 0) dimensions of the Display's DOM element (only updates the DOM
    // element on updateDisplay())
    this.sizeProperty = new TinyProperty( new Dimension2( options.width, options.height ), {
      useDeepEquality: true
    } );

But TinyProperty doesn't take options, looking at the doc, I think this is what would actually be desired:

// @public {Property.<Dimension2>} The (integral, > 0) dimensions of the Display's DOM element (only updates the DOM
    // element on updateDisplay())
    this.sizeProperty = new TinyProperty( new Dimension2( options.width, options.height ) );
    this.sizeProperty.useDeepEquality = true;
    

@jonathanolson does this sound right to you? How bad is this bug? Do we still need useDeepEquality, as it hasn't been implemented here since the scenery conversion from Events.

@zepumph
Copy link
Member Author

zepumph commented Feb 10, 2021

I removed the option above because it was blocking my work in #1131.

@jonathanolson
Copy link
Contributor

It doesn't seem critical, however presumably we'd want useDeepEquality, so that we don't fire off all the changes when the size hasn't actually changed.

It looks like we're not calling it every frame, so we wouldn't see much of a penalty in sims right now.

@zepumph zepumph removed their assignment Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants