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

Element component: mark a batch group as dirty if an attribute has been changed #4289

Merged
merged 3 commits into from
May 31, 2022

Conversation

querielo
Copy link
Contributor

Fixes #2579

MR makes batch group dirty if an element has been changed. So, a user don't have to mark a batch group as dirty manually, because it is error-prone.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@mvaligursky
Copy link
Contributor

Looks great! I'd probably add a function like this to the class

    _dirtyBatch() {
        if (this.batchGroupId !== -1) {
            this.system.app.batcher?.markGroupDirty(this.batchGroupId);
        }
    }

and then use this

        set: function (value) {
            if (this._text && this._text[name] !== value) {
                this._text[name] = value;
                this._dirtyBatch();
            } else if (this._image && this._image[name] !== value) {
                this._image[name] = value;
                this._dirtyBatch();
            }
        }

it would allow us to call _dirtyBatch from multiple places.

@querielo
Copy link
Contributor Author

querielo commented May 31, 2022

Looks great! I'd probably add a function like this to the class

    _dirtyBatch() {
        if (this.batchGroupId !== -1) {
            this.system.app.batcher?.markGroupDirty(this.batchGroupId);
        }
    }

and then use this

        set: function (value) {
            if (this._text && this._text[name] !== value) {
                this._text[name] = value;
                this._dirtyBatch();
            } else if (this._image && this._image[name] !== value) {
                this._image[name] = value;
                this._dirtyBatch();
            }
        }

it would allow us to call _dirtyBatch from multiple places.

@mvaligursky Done

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

looks like a great change to me!

@@ -1667,10 +1673,12 @@ function _define(name) {
return null;
},
set: function (value) {
if (this._text) {
if (this._text && this._text[name] !== value) {
Copy link
Contributor

@jpauloruschel jpauloruschel May 31, 2022

Choose a reason for hiding this comment

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

I don't think it is safe to add this value check before setting the property. There are many setters in both text and image that do specific code depending if the values differ or not, so adding this here will probably break those functionalities.

I think it is best to add the value check difference only to the call to _dirtyBatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: a09bb29

@@ -1668,8 +1674,16 @@ function _define(name) {
},
set: function (value) {
if (this._text) {
if (this._text[name] !== value) {
this._dirtyBatch();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the callback for dirtifying be after the value is changed?

Copy link
Contributor

@mvaligursky mvaligursky May 31, 2022

Choose a reason for hiding this comment

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

it does not matter, this just marks it dirty .. the actual value is not used in this function.
And the batch rebuilds after all scripts are done executing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh ok! All good, then. Thanks, @querielo !

@mvaligursky mvaligursky merged commit f403d7d into playcanvas:main May 31, 2022
@querielo querielo deleted the kirill/element-batching branch May 31, 2022 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ui UI related issue bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update UI image and text elements to work more seamlessly with batch groups
4 participants