Update node.ts setSiblingIndex support negative index #17592

kaokei · 2024-08-31T08:42:45Z

eg.
-1 equals siblings.length -1.
-2 equals siblings.length -2.
-3 equals siblings.length -3.

Re: #

Changelog

Support negative index in setSiblingIndex method

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.

Greptile Summary

This PR updates the setSiblingIndex method in the Node class to support negative indices for more convenient node positioning within the scene graph.

Modifies setSiblingIndex in cocos/scene-graph/node.ts to handle negative indices
Allows positioning nodes relative to the end of the sibling list (e.g. -1 for last position)
Affects core functionality of node positioning and hierarchy management
Requires careful review to ensure consistency with existing behavior
May impact other engine components that rely on sibling indexing

greptile-apps

_{1 file(s) reviewed, 1 comment(s)}
_{Edit PR Review Bot Settings}

cocos/scene-graph/node.ts

github-actions · 2024-08-31T08:51:19Z

Interface Check Report

This pull request does not change any public interfaces !

github-actions · 2024-08-31T09:20:21Z

@kaokei, Please check the result of run test cases:

Test Platform: PR-Test
Editor Version: v3.8.4
Task Result: PASS
Task URL: https://cctest.cocos.org/#/reportview/PR-TEST/0efa4920-11f0-4b93-984b-fc0116d8180b/-1
GitHub Action: https://github.com/cocos/cocos-engine/actions/runs/10643925112

Task Details

github-actions · 2024-08-31T09:20:21Z

@kaokei, Please check the result of run test cases:

Test Platform: PR-Test
Editor Version: v3.8.4
Task Result: PASS
Task URL: https://cctest.cocos.org/#/reportview/PR-TEST/9e61b50a-9604-483f-8f04-f86ac9da1428/-1
GitHub Action: https://github.com/cocos/cocos-engine/actions/runs/10643925112

Task Details

minggo · 2024-09-05T09:40:22Z

It will break compatibility.

kaokei · 2024-09-05T10:26:28Z

It will break compatibility.

@minggo
Is there something I haven't considered?
From my understanding, the original code index = index !== -1 ? index : siblings.length - 1; only accounts for scenarios where the index is a positive number or -1.
However, the new code index = index >= 0 ? index : siblings.length + index; not only supports positive numbers but also supports a wider range of negative numbers.
The new code should encompass the behavior of the old one. Could you provide some cases to illustrate where there are breaking changes?

minggo · 2024-09-06T06:48:19Z

My fault. I doesn't break compatibility.

And this PR just make code more clear, it doesn't support more index range. As the current implementation also supports -2 and -3.

dumganhar · 2024-09-06T13:23:22Z

cocos/scene-graph/node.ts

@@ -630,7 +630,7 @@ export class Node extends CCObject implements ISchedulable, CustomSerializable {
            return;
        }
        const siblings = this._parent._children;
-        index = index !== -1 ? index : siblings.length - 1;
+        index = index >= 0 ? index : siblings.length + index;
        const oldIndex = siblings.indexOf(this);


Would it run well if passing a negative value that smaller than -siblings.length?

@kaokei
Could you take a look at this question?
If index < -siblings.length, for example, it's -siblings.length - 10,
After this logic index = index >= 0 ? index : siblings.length + index;
The index will be siblings.length + (-siblings.length - 10) = -10, -10 will be a invalid index.

It seems splice will check whether the index is in the range and do a clamp operation.

dumganhar · 2024-09-06T13:24:40Z

https://github.com/cocos/cocos-engine/blob/v3.8.5/native/cocos/core/scene-graph/Node.cpp#L347

Also need to modify the cpp code for native platforms.

kaokei · 2024-09-07T05:45:53Z

https://github.com/cocos/cocos-engine/blob/v3.8.5/native/cocos/core/scene-graph/Node.cpp#L347

Also need to modify the cpp code for native platforms.

@dumganhar
Actually, I'm not familiar with C++ code. Could you help me review if this implementation is feasible? The main change is adding logic to force the index to be greater than or equal to 0.

github-actions · 2024-09-07T06:44:41Z

@kaokei, Please check the result of run test cases:

Test Platform: PR-Test
Editor Version: v3.8.4
Task Result: PASS
Task URL: https://cctest.cocos.org/#/reportview/PR-TEST/80ff8528-734e-4134-a628-5cef32eb14be/-1
GitHub Action: https://github.com/cocos/cocos-engine/actions/runs/10748913079

Task Details

github-actions · 2024-09-07T06:44:42Z

@kaokei, Please check the result of run test cases:

Test Platform: PR-Test
Editor Version: v3.8.4
Task Result: PASS
Task URL: https://cctest.cocos.org/#/reportview/PR-TEST/ffb20c1b-d1e7-42e9-a654-50d80b22c16d/-1
GitHub Action: https://github.com/cocos/cocos-engine/actions/runs/10748913079

Task Details

dumganhar · 2024-09-11T06:23:49Z

@dumganhar
Actually, I'm not familiar with C++ code. Could you help me review if this implementation is feasible? The main change is adding logic to force the index to be greater than or equal to 0.

OK, I will be glad to update the cpp code later.

dumganhar · 2024-09-11T06:25:19Z

@kaokei I found you have already updated the cpp code. It looks good.

native/cocos/core/scene-graph/Node.cpp

minggo · 2024-09-11T10:12:26Z

@kaokei could you please send it to v3.8.5? Thanks.

eg. -1 equals siblings.length -1. -2 equals siblings.length -2. -3 equals siblings.length -3.

Force index=0 when index smaller than -siblings.length.

kaokei · 2024-09-12T03:58:21Z

@kaokei could you please send it to v3.8.5? Thanks.

done. @dumganhar

minggo · 2024-09-12T05:52:39Z

@cocos-robot run test cases

github-actions · 2024-09-12T07:16:58Z

@kaokei, Please check the result of run test cases:

Test Platform: PR-Test
Editor Version: v3.8.5
Task Result: FAIL
Task URL: https://cctest.cocos.org/#/reportview/PR-TEST/70f9bb33-0370-44c7-9b22-1163b550efe1/-1
GitHub Action: https://github.com/cocos/cocos-engine/actions/runs/10824986281

Task Details

Platform	build	boot	runned	FailScene
web-mobile	PASS	PASS	FAIL	Hexa,puzzle,shield-node
ios	PASS	PASS	FAIL	Hexa,puzzle,shield-node
mac	PASS	PASS	FAIL	Hexa,puzzle,shield-node

github-actions · 2024-09-12T07:16:58Z

@kaokei, Please check the result of run test cases:

Test Platform: PR-Test
Editor Version: v3.8.5
Task Result: FAIL
Task URL: https://cctest.cocos.org/#/reportview/PR-TEST/ab0dd3a9-b4d0-4b9f-bce2-8e1a8a004242/-1
GitHub Action: https://github.com/cocos/cocos-engine/actions/runs/10824986281

Task Details

Platform	build	boot	runned	FailScene
windows	PASS	PASS	FAIL	Hexa,puzzle,shield-node
android	PASS	PASS	FAIL	Hexa,puzzle,shield-node
wechatgame	PASS	PASS	FAIL	Hexa,puzzle,shield-node

greptile-apps bot reviewed Aug 31, 2024

View reviewed changes

cocos/scene-graph/node.ts Show resolved Hide resolved

dumganhar reviewed Sep 6, 2024

View reviewed changes

kaokei requested a review from dumganhar September 11, 2024 05:32

dumganhar approved these changes Sep 11, 2024

View reviewed changes

minggo reviewed Sep 11, 2024

View reviewed changes

native/cocos/core/scene-graph/Node.cpp Show resolved Hide resolved

minggo approved these changes Sep 11, 2024

View reviewed changes

minggo changed the base branch from v3.8.4 to v3.8.5 September 11, 2024 10:11

minggo changed the base branch from v3.8.5 to v3.8.4 September 11, 2024 10:12

kaokei changed the base branch from v3.8.4 to v3.8.5 September 12, 2024 02:12

kaokei added 2 commits September 12, 2024 11:46

Update node.ts setSiblingIndex support negative index

bcb32b5

eg. -1 equals siblings.length -1. -2 equals siblings.length -2. -3 equals siblings.length -3.

Update Node.cpp setSiblingIndex support negative index

f3164f9

Force index=0 when index smaller than -siblings.length.

kaokei force-pushed the kaokei-patch-1 branch from 32a86c5 to f3164f9 Compare September 12, 2024 03:49

minggo merged commit f7aa23f into cocos:v3.8.5 Sep 12, 2024
24 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update node.ts setSiblingIndex support negative index #17592

Update node.ts setSiblingIndex support negative index #17592

kaokei commented Aug 31, 2024 •

edited by greptile-apps bot

Loading

greptile-apps bot left a comment

github-actions bot commented Aug 31, 2024

github-actions bot commented Aug 31, 2024

github-actions bot commented Aug 31, 2024

minggo commented Sep 5, 2024

kaokei commented Sep 5, 2024

minggo commented Sep 6, 2024

dumganhar Sep 6, 2024 •

edited

Loading

dumganhar Sep 11, 2024

dumganhar Sep 11, 2024

dumganhar commented Sep 6, 2024

kaokei commented Sep 7, 2024

github-actions bot commented Sep 7, 2024

github-actions bot commented Sep 7, 2024

dumganhar commented Sep 11, 2024

dumganhar commented Sep 11, 2024

minggo commented Sep 11, 2024

kaokei commented Sep 12, 2024

minggo commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

Update node.ts setSiblingIndex support negative index #17592

Update node.ts setSiblingIndex support negative index #17592

Conversation

kaokei commented Aug 31, 2024 • edited by greptile-apps bot Loading

Changelog

Continuous Integration

Compatibility Check

Greptile Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 31, 2024

Interface Check Report

github-actions bot commented Aug 31, 2024

Task Details

github-actions bot commented Aug 31, 2024

Task Details

minggo commented Sep 5, 2024

kaokei commented Sep 5, 2024

minggo commented Sep 6, 2024

dumganhar Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

dumganhar Sep 11, 2024

Choose a reason for hiding this comment

dumganhar Sep 11, 2024

Choose a reason for hiding this comment

dumganhar commented Sep 6, 2024

kaokei commented Sep 7, 2024

github-actions bot commented Sep 7, 2024

Task Details

github-actions bot commented Sep 7, 2024

Task Details

dumganhar commented Sep 11, 2024

dumganhar commented Sep 11, 2024

minggo commented Sep 11, 2024

kaokei commented Sep 12, 2024

minggo commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

Task Details

github-actions bot commented Sep 12, 2024

Task Details

kaokei commented Aug 31, 2024 •

edited by greptile-apps bot

Loading

dumganhar Sep 6, 2024 •

edited

Loading