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

Fix the UIOpacity issue when the node is re-added to the scene. #17700

Merged
merged 6 commits into from
Oct 12, 2024

Conversation

yoki0805
Copy link
Contributor

issue:
https://forum.cocos.org/t/topic/159379/745

if (JSB) { render.node._uiProps.localOpacity = render.renderEntity.localOpacity; }
'render.node._uiProps.localOpacity' still needs to be updated on native for fix another issue.
#17503

If possible, it would be great to have this fix synced to v3.8.4 as well.
@minggo

Changelog


Continuous Integration

This pull request:

  • needs automatic test cases check.

    Manual trigger with @cocos-robot run test cases afterward.

  • does not change any runtime related code or build configuration

    If any reviewer thinks the CI checks are needed, please uncheck this option, then close and reopen the issue.


Compatibility Check

This pull request:

  • changes public API, and have ensured backward compatibility with deprecated features.
  • affects platform compatibility, e.g. system version, browser version, platform sdk version, platform toolchain, language version, hardware compatibility etc.
  • affects file structure of the build package or build configuration which requires user project upgrade.
  • introduces breaking changes, please list all changes, affected features and the scope of violation.

fix the render.node._uiProps.localOpacity only update on native.
Copy link

Interface Check Report

This pull request does not change any public interfaces !

@minggo minggo requested a review from qiuguohua October 11, 2024 06:38
@minggo
Copy link
Contributor

minggo commented Oct 11, 2024

@yoki0805 thanks for the PR. As v3.8.4 is releasing, so i can't merge it to v3.8.4.

@yoki0805
Copy link
Contributor Author

@yoki0805 thanks for the PR. As v3.8.4 is releasing, so i can't merge it to v3.8.4.

When v3.8.4 is released, could this issue be included in the known issues list?

@minggo
Copy link
Contributor

minggo commented Oct 11, 2024

It may be released tomorrow. I will add it to known list in release note if we have.

@qiuguohua
Copy link
Contributor

@yoki0805 Sorry I clicked wrong. My suggestion is to add the judgment in _parentChanged.

    protected _parentChanged (): void {
        if (!JSB) {
            return;
        }

The setEntityLocalOpacityDirtyRecursively function should not work on web platforms!

The setEntityLocalOpacityDirtyRecursively function should not work on web platform.
Whenever the node is re-added to the scene, the _parentOpacity value needs to be updated again in the onEnable callback.
@yoki0805
Copy link
Contributor Author

  1. The JSB checking is added.
    protected _parentChanged (): void {
        if (!JSB) {
            return;
        }
  1. Fixed an issue on native platform where a node wasn't affected by its parent's transparency when added to the scene.
  • The node will wait for 2 seconds and then fade out after being added to the scene. (The node is affected by its parent's transparency.)
  • Test demo: UIOpacity.zip

@qiuguohua

@minggo minggo requested a review from qiuguohua October 12, 2024 01:34
@@ -152,7 +154,9 @@ export class UIOpacity extends Component {
// there is a just UIRenderer but no UIOpacity on the node, we should just transport the parentOpacity to the node.
render.renderEntity.localOpacity = parentOpacity;
}
render.node._uiProps.localOpacity = render.renderEntity.localOpacity;
if (JSB) {
Copy link
Contributor

@qiuguohua qiuguohua Oct 12, 2024

Choose a reason for hiding this comment

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

It should be possible to remove it here? @yoki0805

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got what you mean.
UIOpacity.setEntityLocalOpacityDirtyRecursively is only called on native platform.
I will remove the JSB checking here, thank you.

@@ -222,6 +231,10 @@ export class UIOpacity extends Component {
}

public onEnable (): void {
if (this._parentOpacityResetFlag) {
this._parentChanged();
Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to change it to something like the following

        this.node.on(NodeEventType.PARENT_CHANGED, this._parentChanged, this);
        if (this._parentOpacityResetFlag) {
            this._parentChanged();
            this._parentOpacityResetFlag = false;
        } else {
            this.node._uiProps.localOpacity = this._parentOpacity * this._opacity / 255;
            this._setEntityLocalOpacityRecursively(this.node._uiProps.localOpacity);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

public onEnable (): void {
    this.node.on(NodeEventType.PARENT_CHANGED, this._parentChanged, this);
    if (this._parentOpacityResetFlag) {
        this._parentChanged();
        this._parentOpacityResetFlag = false;
    } else {
        this.node._uiProps.localOpacity = this._parentOpacity * this._opacity / 255;
        this._setEntityLocalOpacityRecursively(this.node._uiProps.localOpacity);
    }
}

I changed to that, and I got the different results in test demo.
on web platform:
截圖 2024-10-12 上午10 35 33

on native platform:
截圖 2024-10-12 上午10 36 02

The result on native platform is right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, should also need to set this.node._uiProps.localOpacity.

public onEnable (): void {
    this.node.on(NodeEventType.PARENT_CHANGED, this._parentChanged, this);
    this.node._uiProps.localOpacity = this._parentOpacity * this._opacity / 255;
    if (this._parentOpacityResetFlag) {
        this._parentChanged();
        this._parentOpacityResetFlag = false;
    } else {
        this._setEntityLocalOpacityRecursively(this.node._uiProps.localOpacity);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks good in test demo, thanks!

@qiuguohua qiuguohua requested a review from minggo October 12, 2024 07:29
@minggo minggo merged commit 2ec9087 into cocos:v3.8.5 Oct 12, 2024
12 checks passed
@minggo
Copy link
Contributor

minggo commented Oct 12, 2024

@qiuguohua please update release note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants