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

Jump to bottom

add world-position-changed to CCNode #1728

Closed

knoxHuang wants to merge 1 commit into cocos:v1.5 from knoxHuang:v1.5

Contributor

knoxHuang commented Jun 12, 2017 •

edited

Loading

Re: cocos-creator/fireball#5889

Changes proposed in this pull request:

添加 ‘world-position-changed’ 事件

@cocos-creator/engine-admins

zilongshanren reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

@@ @@ -442,6 +447,9 @@ var Node = cc.Class({ @@
                                               this.emit(POSITION_CHANGED);
                                           }
                                       }

Contributor

zilongshanren Jun 12, 2017

每次节点坐标变化都发这个事件？这样会卡爆吧？

jareguo suggested changes

View reviewed changes

Contributor

jareguo left a comment

这个开销太大了，而且 setPosition, setPositionX 什么的 API 也没发送消息

Contributor

nantas commented Jun 12, 2017

加这个功能是为了解决什么问题？

Contributor

jareguo commented Jun 12, 2017 •

edited

Loading

解决粒子，MotionStreak和物理模块模块无法追踪到父节点的坐标更改的问题。这些模块现在必须放在场景根节点下，否则父节点移动的话效果就错了。

Contributor

zilongshanren commented Jun 12, 2017

@jareguo 加这个功能不怕卡？肯定会影响游戏性能的。。。

Contributor Author

knoxHuang commented Jun 12, 2017

肯定不是现在的写法，现在只是给 jare 看看而已，性能的不能太耗

Contributor

jareguo commented Jun 12, 2017

加这个功能不怕卡？肯定会影响游戏性能的。。。

有不卡的写法啊，让 Knox 试试，万一写出来了，那不是很爽？这个功能做游戏很实用。

Contributor

zilongshanren commented Jun 12, 2017

@jareguo 这种有可能影响引擎性能的特性，要尽量少加，能不加就不加。。。游戏开发可以用其他方式来实现同样的功能。

Contributor

jareguo commented Jun 12, 2017

这种有可能影响引擎性能的特性，要尽量少加，能不加就不加。。。游戏开发可以用其他方式来实现同样的功能。

如果有完全零开销的实现方式呢？那就可以加了吧？

Contributor

zilongshanren commented Jun 12, 2017

@jareguo 恩，可以。

knoxHuang force-pushed the v1.5 branch from f0f02be to 783d6f5 Compare

June 13, 2017 03:32

Contributor Author

knoxHuang commented Jun 13, 2017 •

edited

Loading

@jareguo @zilongshanren 改好了你们有空看看

jareguo suggested changes

View reviewed changes

cocos2d/core/CCNode.js Outdated

@@ @@ -1219,6 +1282,8 @@ var Node = cc.Class({ @@
                       this._checkTouchListeners();
                       this._checkMouseListeners();
+                      this._removeWorldPositionChanged();

Contributor

jareguo Jun 13, 2017

targetOff 不需要这样吧，就算调用了这个方法，里面 hasListener 的判断都过不去。

Contributor Author

knoxHuang Jun 13, 2017 •

edited

Loading

过得去吧，在这个之前不是执行了 this._EventTargetTargetOff(target); ?

Contributor

jareguo Jun 13, 2017

哦，我没看到~ 但这也还是不需要调用 _removeWorldPositionChanged 啊？

Contributor Author

knoxHuang Jun 13, 2017

移除节点是不会走 off ，只会走 targetOff，如果不在这里执行，那你要如何 off 掉事件？

cocos2d/core/utils/base-node.js Outdated

@@ @@ -384,6 +384,8 @@ var BaseNode = cc.Class({ @@
                       else if (value) {
                           this._onHierarchyChanged(null);
                       }
+                      this.emit('parent-changed', oldParent);

Contributor

jareguo Jun 13, 2017

用宏

jareguo reviewed

View reviewed changes

test/qunit/unit-es5/test-world-position.js Outdated

		@@ -0,0 +1,147 @@
		module('world-position-changed');

		test('world position changed for parnet', function () {

Contributor

jareguo Jun 13, 2017

world-position-changed should be emitted by parent

jareguo reviewed

View reviewed changes

test/qunit/unit-es5/test-world-position.js Outdated

+                  CC.parent = BB;
+                  DD.parent = CC;
+                  EE.parent = DD;
+                  FF.parent = EE;

Contributor

jareguo Jun 13, 2017

不用嵌套这么深…… 三层就够了，三层可以没理由 5 层不行

jareguo reviewed

View reviewed changes

test/qunit/unit-es5/test-world-position.js Outdated

+                  var called = false;
+                  var _onWorldPositionChangedCallback = function () {
+                      called = true;
+                  };

Contributor

jareguo Jun 13, 2017

把这个闭包替换成
var _onWorldPositionChangedCallback = new Callback().enable();

jareguo reviewed

View reviewed changes

test/qunit/unit-es5/test-world-position.js Outdated


		AA.position = cc.p(100, 100);

		ok(called, 'FF receives world-position-changed event for parent position changed');

Contributor

jareguo Jun 13, 2017

然后这里改成 _onWorldPositionChangedCallback.once('world-position-changed emitted by ?which? parent when position changed');

jareguo reviewed

View reviewed changes

test/qunit/unit-es5/test-world-position.js Outdated

+                  FF.off('world-position-changed', _onWorldPositionChangedCallback);
+                  called = false;
+                  EE.on('world-position-changed', _onWorldPositionChangedCallback);

Contributor

jareguo Jun 13, 2017

不要反复修改监听的子节点，而是在同一个子节点上监听，父亲、祖父、曾祖父上的各种事件

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

+              var WORLD_POSITION_CHANGED = 'world-position-changed';
+              var PARENT_CHANGED = 'parent-changed';
               var CHILD_ADDED = 'child-added';

Contributor

jareguo Jun 13, 2017

WORLD_POSITION_CHANGED 放在 POSITION_CHANGED 后一行

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

		var WORLD_POSITION_CHANGED = 'world-position-changed';

		var PARENT_CHANGED = 'parent-changed';

Contributor

jareguo Jun 13, 2017

PARENT_CHANGED 之后没必要空一行吧…… 和 CHILD_ADDED 是同一批事件

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

+                      // remove oldParent event
+                      var oldParent = event.detail;
+                      if (oldParent) {
+                          oldParent.off(WORLD_POSITION_CHANGED, oldParent._onWorldPositionChanged, this);

Contributor

jareguo Jun 13, 2017

传入的参数是 oldParent._onWorldPositionChanged 和 this，这逻辑说不通啊？
要获取的是 oldParent 示例上的成员方法，但是 this 又是 this…… 这是什么逻辑？

Contributor Author

knoxHuang Jun 13, 2017

这里应该是写 oldParent.off(WORLD_POSITION_CHANGED, this._onWorldPositionChanged, this);

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

+                      if (oldParent) {
+                          oldParent.off(WORLD_POSITION_CHANGED, oldParent._onWorldPositionChanged, this);
+                          oldParent.off(PARENT_CHANGED, oldParent._onParentChanged, this);
+                          oldParent._removeWorldPositionChanged();

Contributor

jareguo Jun 13, 2017

oldParent.off(WORLD_POSITION_CHANGED) 时，本来就会触发 oldParent._removeWorldPositionChanged 了，这里不用重复调用

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

-                          oldParent.off(PARENT_CHANGED, oldParent._onParentChanged, this);
-                          oldParent._removeWorldPositionChanged();
+                          oldParent.off(WORLD_POSITION_CHANGED, this._onWorldPositionChanged, this);
+                          oldParent.off(PARENT_CHANGED, this._registerWorldPosChangedToNewParent, this);

Contributor

jareguo Jun 13, 2017

都没 on，怎么就 off 了

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

@@ @@ -2576,6 +2560,10 @@ if (CC_JSB) { @@
                * @param {Vec2} event.detail - The old position, but this parameter is only available in editor!
                */
               /**
+               * @event world-position-changed
+               * @param {Event.EventCustom} event

Contributor

jareguo Jun 13, 2017

又没有参数…… 把 event 去掉吧

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js

@@ @@ -2585,6 +2573,10 @@ if (CC_JSB) { @@
                * @param {Event.EventCustom} event
                */
               /**
+               * @event parent-changed
+               * @param {Event.EventCustom} event

Contributor

jareguo Jun 13, 2017

有参数啊，oldParent

jareguo reviewed

View reviewed changes

cocos2d/core/CCNode.js Outdated

    
                          parent.off(SCALE_CHANGED, this._onWorldPositionChanged, this);

                          parent.onoff(register, WORLD_POSITION_CHANGED, this._onWorldPositionChanged, this);

                          parent.onoff(register, ROTATION_CHANGED, this._onWorldPositionChanged, this);

                          parent.onoff(register, SCALE_CHANGED, this._onWorldPositionChanged, this);

Contributor

jareguo Jun 13, 2017 •

edited

Loading

上面那三行代码出现了三次…… 提取一下吧？然后这里就 this._doRegisterWPCTo(parent, register);


          add world-position-changed event to CCNode

558402d

add onoff to event-target
add parent-changed event  to CCNode
add test-world-position to test

knoxHuang force-pushed the v1.5 branch from b6d3328 to 558402d Compare

June 13, 2017 08:20

jareguo approved these changes

View reviewed changes

Contributor

jareguo left a comment

LGTM，本月新的最佳 PR 诞生了

zilongshanren reviewed

View reviewed changes

cocos2d/core/event/event-target.js

+               *                              When false, callback will NOT be invoked when event's eventPhase attribute value is CAPTURING_PHASE.
+               *                              Either way, callback will be invoked when event's eventPhase attribute value is AT_TARGET.
+               */
+              proto.onoff = function (isOn, type, callback, target, useCapture) {

Contributor

zilongshanren Jun 13, 2017

这个 api 太扯了吧。。。 onoff 使用成本比单独使用 On或者 off 都要高。。。而且这个函数一下子就 5 个参数了。。

Contributor

jareguo Jun 13, 2017 •

edited

Loading

不然你有其它建议？比如 CCButton 中，

            this.node.off(cc.Node.EventType.TOUCH_START, this._onTouchBegan, this);
            this.node.off(cc.Node.EventType.TOUCH_MOVE, this._onTouchMove, this);
            this.node.off(cc.Node.EventType.TOUCH_END, this._onTouchEnded, this);
            this.node.off(cc.Node.EventType.TOUCH_CANCEL, this._onTouchCancel, this);

            this.node.off(cc.Node.EventType.MOUSE_ENTER, this._onMouseMoveIn, this);
            this.node.off(cc.Node.EventType.MOUSE_LEAVE, this._onMouseMoveOut, this);

完全和 _registerEvent 重复的代码，你不觉得很奇怪吗？
如果有了 onoff，你只要定义 _registerEvent 成

    _registerEvent: function (register) {
        this.node.onoff(register, cc.Node.EventType.TOUCH_START, this._onTouchBegan, this);
        this.node.onoff(register, cc.Node.EventType.TOUCH_MOVE, this._onTouchMove, this);
        this.node.onoff(register, cc.Node.EventType.TOUCH_END, this._onTouchEnded, this);
        this.node.onoff(register, cc.Node.EventType.TOUCH_CANCEL, this._onTouchCancel, this);

        this.node.onoff(register, cc.Node.EventType.MOUSE_ENTER, this._onMouseMoveIn, this);
        this.node.onoff(register, cc.Node.EventType.MOUSE_LEAVE, this._onMouseMoveOut, this);
    },

Contributor

zilongshanren Jun 13, 2017

@jareguo 一点也不奇怪啊， register 和 unregister 一看函数名字就知道是干啥的。
重复代码不算呀，里面调用的 api 不一样。关键是你多了一个 onoff，以后大家可能就都会用这个 api 了，添加 api 需要谨慎。另外，这种 onoff 的封装，其他开发者可以自己做，没必要引擎提供。

至于包体积，这里差不了 0.001kb 吧？

Contributor

jareguo Jun 13, 2017

问题是你没 unregister 这个方法啊，你没提供 onoff 的话，大家就不会下意识的写出可靠、简洁的代码。否则你 CCButton 为什么不写成 unregister ？这个 API 可能大家都会用到，那用就用啊，引擎不就是鼓励大家尽量少犯错吗？

Contributor

zilongshanren Jun 13, 2017

@jareguo Button那边是需要封装一个 unregister 的，之前写的不是很好。
我主要是觉得这个函数的参数数量有点吓人。

Contributor

jareguo Jun 13, 2017

你会这么纠结函数参数数量的人，都能不小心“写得不是很好”，换做其它人也难免一样。那不就是说明了 onoff 的必要吗…… 如果有 onoff，大家一开始就能写得很好看。
至于参数数量，这个应该也还好吧…… 一般只用写前 4 个

Contributor

pandamicro Jun 13, 2017

我同意子龙的观点，这个 API 太怪了

没有提供任何新的功能
指意不明，一眼看上去都不知道是在 on 还是在 off，代码阅读起来也有障碍
如果要节省包体，用户完全可以自己封装一个类似的函数，引擎提供会给用户更多困扰，什么时候该用 onoff，什么时候该用 on & off？
我也不认为提供了 onoff 对用户的代码有什么指导作用，毕竟用户需要写成 registerEvent 这种形式才能节省代码，然而用户很多时候写的代码都是 on / off 直接混在各种 onEnable / onDisable 代码中，甚至不想干的外部代码中。

其实从 API 形式上，我认为任何通过 flag 来区分功能的 API 都很可能是坏的设计，它把功能隐含在参数中，导致表意不清，功能混杂

zilongshanren reviewed

View reviewed changes

cocos2d/core/CCNode.js

@@ @@ -1204,6 +1246,10 @@ var Node = cc.Class({ @@
                       else if (_mouseEvents.indexOf(type) !== -1) {
                           this._checkMouseListeners();
                       }
+                      if (type === WORLD_POSITION_CHANGED) {
+                          this._registerWPosChanged(false);

Contributor

zilongshanren Jun 13, 2017

直接使用 register 和 unregister 不好一些吗？ @jareguo 为什么一定要通过参数来实现一个函数的多种行为呢。。

Contributor

zilongshanren Jun 13, 2017

@jareguo @knoxHuang 我比较倾向于一个函数只做一件事情。

Contributor

jareguo Jun 13, 2017

代码少了，包体就小了，出错的几率就小了（严格保证 on 和 off 互相配对）

Contributor

jareguo Jun 13, 2017

比如上面说的 CCButton 的问题，你怎么保证 onDisable 时 off 的事件，完全和 registerEvent 中的匹配？换一个人来维护，可能不小心就弄错了

Contributor

zilongshanren Jun 13, 2017 •

edited

Loading

@jareguo 并不能严格保证 on 和 off 互相配对吧，还是要通过传 true 和 false 来保证配对

Contributor

jareguo Jun 13, 2017 •

edited

Loading

这不就够了吗？你换成自动 off 的机制，只会带来复杂度或者内存占用的上升

Contributor

zilongshanren Jun 13, 2017

@jareguo 我不是说要自动 off，我是说手动去调用 off，就像现在你要调用 onoff(false)一样。

Contributor

jareguo Jun 13, 2017

不一样啊，你原来代码写两处，现在写一处，就能保证 off 的数量和参数一致。比如 CCButton 那边，6 个事件，每个事件参数都不一样，你写两边不蛋疼吗…… 如果另一个哪天改了 on 或 off，就有可能忘了同步另一边的 off 或 on，毕竟你是分开写的。

Contributor

zilongshanren Jun 13, 2017

@jareguo 参考 H5 的 DOM 的 api 吧，好像没有 onoff?

Contributor

pandamicro Jun 13, 2017

比如上面说的 CCButton 的问题，你怎么保证 onDisable 时 off 的事件，完全和 registerEvent 中的匹配？换一个人来维护，可能不小心就弄错了

我觉得咱们不能这么惯开发者，更不能这么惯自己的研发，维护过程中可能产生问题的地方多了去了，重要的是相信开发者的自身素养，当然，也要相信，能力有限的开发者，你再保护他也会犯一样多的错误。比如他仍然可能在不同的地方对不同的事件调用 onoff 导致不匹配而不是使用你所展示的封装。

另外，反对这种理念的另一个原因是，如果开发者自己把事件匹配错了，他不会怪引擎的，明显是自己的原因，所以这种帮衬是一种越俎代庖式的多余

zilongshanren reviewed

View reviewed changes

cocos2d/core/CCNode.js

+                  _registerWPosChangedToNewParent (event) {
+                      // remove oldParent event
+                      if (event.detail) {
+                          this._doRegisterWPosChangedTo(event.detail, false);

Contributor

zilongshanren Jun 13, 2017

_doRegisterWPosChangedTo 和 _onWPosChanged封装感觉把代码逻辑变复杂了。。。因为函数名字不好看，不能一眼看出这个函数的意图。不如直接 on/off 来得清晰。

Contributor

jareguo Jun 13, 2017

_onWPosChanged 里面就一行代码，还能怎么改……
_doRegisterWPosChangedTo 你希望怎么改？

Contributor

zilongshanren Jun 13, 2017

@jareguo _onWPosChanged 不要啊，直接 emit 就可以了，emit 的意图很清晰了。
_doRegisterWPosChangedTo 这个名字你说是啥意思？还要结合函数参数。。。而且必须读里面的代码才知道这个函数是干啥的。我建议改成两个函数，一个是 registerWorldPosChanged，一个是 unregisterWorldPosChanged
WPos 这是啥简写啊。。。

Contributor

jareguo Jun 13, 2017

_onWPosChanged 不要啊，直接 emit 就可以了，emit 的意图很清晰了。

这是回调函数啊，怎么直接 emit

WPos 这是啥简写啊。。。

这是为了统一缩写，因为有 _registerWPosChangedToNewParent 这个方法，所以 WorldPosition 简写成 WPos，这里为了保持统一。

pandamicro suggested changes

View reviewed changes

Contributor

pandamicro left a comment

当然，比起 API 的担忧，我同样担忧这个事件的用例。这个功能反映的是对 world transform 随时跟踪的需求，这种需求我觉得可以考虑从 renderer 层面提供，renderer 是最直接可以获知 world transform 修改以及获取新值的。目前的实现看上去太过于复杂了

cocos2d/core/event/event-target.js

+               *                              When false, callback will NOT be invoked when event's eventPhase attribute value is CAPTURING_PHASE.
+               *                              Either way, callback will be invoked when event's eventPhase attribute value is AT_TARGET.
+               */
+              proto.onoff = function (isOn, type, callback, target, useCapture) {

Contributor

pandamicro Jun 13, 2017

我同意子龙的观点，这个 API 太怪了

没有提供任何新的功能
指意不明，一眼看上去都不知道是在 on 还是在 off，代码阅读起来也有障碍
如果要节省包体，用户完全可以自己封装一个类似的函数，引擎提供会给用户更多困扰，什么时候该用 onoff，什么时候该用 on & off？
我也不认为提供了 onoff 对用户的代码有什么指导作用，毕竟用户需要写成 registerEvent 这种形式才能节省代码，然而用户很多时候写的代码都是 on / off 直接混在各种 onEnable / onDisable 代码中，甚至不想干的外部代码中。

其实从 API 形式上，我认为任何通过 flag 来区分功能的 API 都很可能是坏的设计，它把功能隐含在参数中，导致表意不清，功能混杂

cocos2d/core/CCNode.js

@@ @@ -1204,6 +1246,10 @@ var Node = cc.Class({ @@
                       else if (_mouseEvents.indexOf(type) !== -1) {
                           this._checkMouseListeners();
                       }
+                      if (type === WORLD_POSITION_CHANGED) {
+                          this._registerWPosChanged(false);

Contributor

pandamicro Jun 13, 2017

比如上面说的 CCButton 的问题，你怎么保证 onDisable 时 off 的事件，完全和 registerEvent 中的匹配？换一个人来维护，可能不小心就弄错了

我觉得咱们不能这么惯开发者，更不能这么惯自己的研发，维护过程中可能产生问题的地方多了去了，重要的是相信开发者的自身素养，当然，也要相信，能力有限的开发者，你再保护他也会犯一样多的错误。比如他仍然可能在不同的地方对不同的事件调用 onoff 导致不匹配而不是使用你所展示的封装。

另外，反对这种理念的另一个原因是，如果开发者自己把事件匹配错了，他不会怪引擎的，明显是自己的原因，所以这种帮衬是一种越俎代庖式的多余

Contributor

pandamicro commented Jun 13, 2017

具体的建议我想想再说

Contributor

jareguo commented Jun 13, 2017

这个功能反映的是对 world transform 随时跟踪的需求，这种需求我觉得可以考虑从 renderer 层面提供，renderer 是最直接可以获知 world transform 修改以及获取新值的。目前的实现看上去太过于复杂了

这个我下午和子龙讨论过了，renderer 层来实现的话，问题也有

所有节点每次 transformDirty 时，都要 check world-position-changed 事件是否有注册，开销更大
用户监听的回调中，有可能继续修改场景树，导致正在进行的 render 过程受到影响，导致渲染出错。
render 层要保证在 clear dirty flag 之前触发这个事件，太早或太晚都不行，这样需要 render 层对 dirty flag 有统一的 clear 方式，而且将来不能出错

目前的实现其实也就 40 几行代码，相比耦合 render 层，我觉得思路更清晰。（毕竟 render 要考虑的平台有三个啊！）

Contributor

jareguo commented Jun 13, 2017

我同意子龙的观点，这个 API 太怪了

没有提供任何新的功能

这个 API 其实就是语法糖，你说有提供新功能吗？没有，但是语法糖有价值吗？当然有。

指意不明，一眼看上去都不知道是在 on 还是在 off，代码阅读起来也有障碍

命名可以再优化

如果要节省包体，用户完全可以自己封装一个类似的函数，引擎提供会给用户更多困扰，什么时候该用 onoff，什么时候该用 on & off？

需要 off 的时候几乎都能替换成 onoff

其实从 API 形式上，我认为任何通过 flag 来区分功能的 API 都很可能是坏的设计，它把功能隐含在参数中，导致表意不清，功能混杂

照你这么说，setVisible 应该去掉，改成 show 和 hide

Contributor

pandamicro commented Jun 13, 2017

这个 API 其实就是语法糖，你说有提供新功能吗？没有，但是语法糖有价值吗？当然有。

这就是争议点，很显然不是所有语法糖都有价值，而这个就属于我认为没有任何价值的。

照你这么说，setVisible 应该去掉，改成 show 和 hide

当然不是，这完全不是一个概念，一个是设置 visible 状态，设置的主体本身就是一个 boolean，而 onoff 是通过一个 flag 参数来变化其功能，这个 API 的坏味道太明显了

我不是很理解你的坚持，这个 API 的坏味道在我和子龙看来都是很明显的，他不能从根本上解决用户的问题，带给用户更多选择，还要求用户学习某种特殊的封装写法（如果组件或类中没有 registerEvent(flag) 类似的写法，根本没有价值，用户只是将所有 on / off 都改为 onoff，明显更难阅读），即使用户学会并习惯了 registerEvent 的包装方法，依然要保证记得正确匹配 registerEvent(true) 和 registerEvent(false)，这在我看来跟一组 on / off 的匹配没有本质差别

Contributor

jareguo commented Jun 13, 2017 •

edited

Loading

这个 API 的坏味道在我和子龙看来都是很明显的，他不能从根本上解决用户的问题，带给用户更多选择，还要求用户学习某种特殊的封装写法（如果组件或类中没有 registerEvent(flag) 类似的写法，根本没有价值，用户只是将所有 on / off 都改为 onoff，明显更难阅读）

子龙那边已经没有问题了，我和他后面讨论过他就明白了。
我坚持的不是一定要这么改，而是这么改能解决问题。问题是什么我解释了，怎么解决的我也提供了。你也觉得原来那样写有问题，但你也一时拿不出更好的方案。而我现在就有一个现成的，没有多余内存和GC开销的，非常轻量的方案。这怎么会是没有价值的语法糖呢？

我再重申一下前面的观点：

需要 off 的时候几乎都能替换成 onoff

这就是这个 API 的核心功能，只要你需要 off，那就能合并 on 和 off。当然你代码少不合并也行，我只是提供了一个合并的可能性。

我也不认为提供了 onoff 对用户的代码有什么指导作用，毕竟用户需要写成 registerEvent 这种形式才能节省代码，然而用户很多时候写的代码都是 on / off 直接混在各种 onEnable / onDisable 代码中，甚至不想干的外部代码中。

之前没有这个 API，所以用户当然原始的写法手动撸了。如果你有了这个 API，用户慢慢就会懂得如何写出更优雅的代码。而不是 on / off 混写。像 CCButton 这样有坏味道的组件非常多，我再举一个 CCLayout 的例子

这里面有三次 on / off，实际上做的是同一件事情。正常开发者看到这样的代码，会觉得有坏味道，那么重构的过程中，自然就会先把前 8 行 on 的代码提取成一个 registerEvent 方法。提取出来后，registerEvent 和下面的 4 行 off，仍然是有冗余的部分，那他很自然会想到，我应该把 register 和 deregister 写在一起，这样能保证注册和反注册时操作是成对的。

此时如果有 onoff 方法，他很自然就会想到应该重构成这个 PR 里的写法，只用一个 register 方法就搞定了。这个例子就回答了 onoff 的指导作用，也回答了为什么用户会写一个 registerEvent 的这种形式。

Contributor

jareguo commented Jun 13, 2017 •

edited

Loading

他不能从根本上解决用户的问题，带给用户更多选择，还要求用户学习某种特殊的封装写法（如果组件或类中没有 registerEvent(flag) 类似的写法，根本没有价值，用户只是将所有 on / off 都改为 onoff，明显更难阅读）

再举一个例子，CCScrollView（我真的是随便找的，一找一个准，以码农的尊严保证没骗你。）

这里原来就定义了 registerEvent 和 unregisterEvent 了，说明这个需求不是我 YY 的。现在有了 onoff，就可以直接合并这两个方法。
然后调用的时候，_registerEvent() 改成 _registerEvent(true)，_unregisterEvent 改成 _registerEvent(false)，很影响阅读吗？想比起维护两段一模一样的代码，加一个 true / false 我觉得更靠谱。

现在有三个例子（CCButton / CCLayout / CCScrollView）摆在你面前了，或者你试试提供一个更轻量、无 GC 负担的方案来解决这里的坏味道？

Contributor

pandamicro commented Jun 14, 2017 •

edited

Loading

现在有三个例子（CCButton / CCLayout / CCScrollView）摆在你面前了，或者你试试提供一个更轻量、无 GC 负担的方案来解决这里的坏味道？

我完全不觉得 register / unregister 的写法有任何的坏味道，多写几行代码的事情，但是保障代码逻辑是清晰的，你用你的思考回路来代替所有用户的思路说服不了我，当用户想法跟你一样时，自己做这层封装没有任何难度和成本，这种情况下难道引擎做这件事情不是越俎代庖么？如果用户想法跟你不一样，还要像我一样忍受这种 API 设计带来的痛苦和困扰。

而是这么改能解决问题。问题是什么我解释了，怎么解决的我也提供了。你也觉得原来那样写有问题，但你也一时拿不出更好的方案。而我现在就有一个现成的，没有多余内存和GC开销的，非常轻量的方案。这怎么会是没有价值的语法糖呢？

我抗议这里在偷换概念，我们现在讨论的是 onoff 的作用，而不是这个 PR 的作用，你说的问题和问题的解决方案是关于如何跟踪 global transform 改变的，这个问题我还没有开始讨论。onoff 只是语法糖，并不解决任何问题，只是为了减少代码行数。

逻辑代码组织是用户的工作，我们的核心工作是保障引擎本身的性能，健壮性和功能完整性，其他都只是 bonus

Contributor

pandamicro commented Jun 14, 2017

关于 global transform 的改变，我的直觉对于现在的实现有疑问是因为，global transform 信息不属于 node，它是渲染层信息，只跟渲染相关，而提供 global transform change 事件是将组件的逻辑代码依赖到渲染层了，有混淆逻辑和渲染的嫌疑。更严重的是，当我们在逻辑树的 Node 层面实现这种逻辑的时候，可能是在耦合渲染层和逻辑层，逻辑树的 Node 不应该去代理渲染层的信息，更不应该通知渲染层的状态改变。

global transform 是渲染层的核心概念，只有 render command 才保有这个信息，目前他没有直接被暴露到任何其他地方，只在渲染层计算和上传 GPU 数据时用到，SGNode 的获取&计算 transform API 也都是通过 render command 暴露的 API 来计算的。这种情况下我才会认为在 Node 层通知这样一个事件是多么诡异

Contributor

jareguo commented Jun 14, 2017

我完全不觉得 register / unregister 的写法有任何的坏味道，多写几行代码的事情，但是保障代码逻辑是清晰的。onoff 只是语法糖，并不解决任何问题，只是为了减少代码行数。如果用户想法跟你不一样，还要像我一样忍受这种 API 设计带来的痛苦和困扰。

我好像从来没提到过代码行数的问题吧？我最开始说了，onoff 的作用是鼓励写出可维护的代码。这个你不认可就算了， @knoxHuang 把 onoff 去掉吧

Contributor

jareguo commented Jun 14, 2017

global transform 信息不属于 node，它是渲染层信息，只跟渲染相关。global transform 是渲染层的核心概念。

这个出发点的理论依据是什么？这里提供给用户的是 world position，没有帮用户算出值是多少，只是通知用户这个值 dirty 了。

pandamicro reviewed

View reviewed changes

cocos2d/core/CCNode.js

+                  },
+                  _doRegisterWPosChangedTo (target, register) {
+                      target.onoff(register, WORLD_POSITION_CHANGED, this._onWPosChanged, this);

Contributor

pandamicro Jun 14, 2017

这里注册 parent 的 WORLD_POSITION_CHANGED，然后回调中在自己身上 emit WORLD_POSITION_CHANGED 的原因是什么？

另外，原始 parent 的 WORLD_POSITION_CHANGED 我没有看到被发出的地方

Contributor

pandamicro Jun 14, 2017 •

edited

Loading

看了一下大概理解了，这里应该监听的是父节点的 POSITION_CHANGED，所以在这里 emit WORLD_POSITION_CHANGED 并不是很严谨，因为 global transform 还没有计算，父节点
TRS 的修改不意味着子节点的 world transform 一定会改变。

举个例子，开放了这个事件给用户之后，他可能会监听这个事件，然后在事件中用 node.getNodeToWorldTransform() 来获取 world transform 做计算，然而这个事件的时机导致它获取的 world transform 并不是当前帧的正确值

感觉是不是应该叫 PARENT_TRS_CHANGED？虽然这个意义也很奇怪，不过表述是准确的。

Contributor Author

knoxHuang Jun 14, 2017 •

edited

Loading

这个上面的 _registerWPosChanged 函数中

this.onoff(register, POSITION_CHANGED, this._onWPosChanged, this);

当节点发生坐标变换的时候，就回去 emit WORLD_POSITION_CHANGED 消息

目前是节点遇到 position, scale, rotation, parent 改变时，回去 emit WORLD_POSITION_CHANGED 消息

Contributor

pandamicro Jun 14, 2017

但是你现在的实现监听的是父节点的 WORLD_POSITION_CHANGED 事件

Contributor

pandamicro commented Jun 14, 2017

这个出发点的理论依据是什么？这里提供给用户的是 world position，没有帮用户算出值是多少，只是通知用户这个值 dirty 了。

我一开始以为真的是通知 world transform 变化，不过看下来应该不是，那么之前的问题没必要纠结，这里的通知不涉及 world transform 的状态

Contributor

jwu commented Jun 14, 2017 •

edited

Loading

看了一下你们 onoff 的争论，我的疑问点主要是：

有争议的 API 为什么是在 review 阶段才讨论，而不是实现之前就确定清楚？
出现争议的时候，最后谁拍板决定？或者有什么方法结束争议？
是否有明确参考标准，比如 API 出现争议的时候，我们首先参考哪些其他同类产品？（我根据 onoff 这个 API 去翻阅了，EventEmiiter3, Pixi, Phaser, PlayCanvas, Babylon 和 DOM，发现他们均没有提供同等功能的 API，也许这个可以作为一个参考线?）

Contributor

jareguo commented Jun 14, 2017

node.getNodeToWorldTransform() 来获取 world transform 做计算，然而这个事件的时机导致它获取的 world transform 并不是当前帧的正确值

怎么不会是正确值？这里收到事件后， sgNode 的 setPosition 操作已经完成了。sgNode 的 renderCmd 的 dirtyFlag 已经设置了。如果此时 getNodeToWorldTransform 就会重新进行一次 transform 计算，所以能拿到当前帧的最新值。（就算拿不到也是底层应该修复的 bug）

Contributor

jareguo commented Jun 14, 2017

感觉是不是应该叫 PARENT_TRS_CHANGED？虽然这个意义也很奇怪，不过表述是准确的。

从实现上来看，那对用户来说事件名就应该是 world-transform-changed。这样也行。否则就会多一个事件出来 parent-position-changed + world-transform-changed

Contributor

jareguo commented Jun 14, 2017 •

edited

Loading

有争议的 API 为什么是在 review 阶段才讨论，而不是实现之前就确定清楚？

这是我的锅，之前 PR 的实现里代码冗余比较多，所以临时提议的。后来重构过冗余少了就看不出问题，所以不加这个 API 也没事。

出现争议的时候，最后谁拍板决定？或者有什么方法结束争议？

没有客户端主程或编辑器主程，一般就是相互妥协，或者王楠决定

是否有明确参考标准，比如 API 出现争议的时候，我们首先参考哪些其他同类产品？（我根据 onoff 这个 API 去翻阅了，EventEmiiter3, Pixi, Phaser, PlayCanvas, Babylon 和 DOM，发现他们均没有提供同等功能的 API，也许这个可以作为一个参考线?）

我一向都会去翻各个引擎的 API 作为参考。这里我没拿同类产品说事主要是因为同类产品没有 off 这个 API，所以他们很难设计出 addOrRemoveListener 这样的接口。还有就是他们很多是采用“信号槽”这样的 JS 委托机制，不然就是没有 onEnable / onDisable 这类生命周期回调，所以需求没那么强。

Contributor

pandamicro commented Jun 14, 2017

看了一下 getNodeToWorldTransform 有算，但是由于这是当前帧逻辑过程中发出的事件，不一定是逻辑过程结束后的值，所以实际上跟用户预期可能还是不同。另外，用户如果监听这个事件，而一帧当中对 TRS 的操作比较频繁的话，会大大增加计算的损耗。

我觉得我们需要重新审核一下 POSITION_CHANGED / ROTATION_CHANGED / SCALE_CHANGED 此类事件的性能损耗，以及放在 CCNode 中的必要性。

@jwu 说的对，需要规范一下 API 的提出和 review，以及审核机制

之前 TRS 的事件添加应该是在初期我们的引擎功能还很不完善的时候做的，那个阶段可能没有那么多精力去审核所有的 API，所以环节上有所缺失。

@nantas @linshun

Contributor

jareguo commented Jun 14, 2017 •

edited

Loading

看了一下 getNodeToWorldTransform 有算，但是由于这是当前帧逻辑过程中发出的事件，不一定是逻辑过程结束后的值，所以实际上跟用户预期可能还是不同。另外，用户如果监听这个事件，而一帧当中对 TRS 的操作比较频繁的话，会大大增加计算的损耗。

我觉得我们需要重新审核一下 POSITION_CHANGED / ROTATION_CHANGED / SCALE_CHANGED 此类事件的性能损耗，以及放在 CCNode 中的必要性。

其实 world-position-changed 实现上和 position-changed 没区别，如果你觉得都不合适，那你就另开 issue 讨论吧，把你觉得合理的做法提出来，看看在实现、性能上是否能取代现有需求。

Contributor

pandamicro commented Jun 14, 2017

嗯，麻烦先 pending 一下这个 PR，我会给出一个提案

jareguo added Discussion PR: Do not merge labels

knoxHuang closed this

knoxHuang deleted the v1.5 branch

February 9, 2018 03:37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Discussion PR: Do not merge