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

Scrollview remove entity reference #7206

Merged

Conversation

Maksims
Copy link
Collaborator

@Maksims Maksims commented Dec 17, 2024

This PR depends on #7194 and #7204 (and includes them already), merge them first please so diff is more concise.

This PR does not change API or behavior.

As mentioned in #7195 - EntityRefence need to go. This PR completes that, by removing a last dependency on it and also removing reliance on ComponentSystem's global event lists, which leads to exponentially higher costs of creating/removing related components (Element especially).

There is a significant performance improvement when creating/removing such components similar to other two PR's mentioned above. This also benefits ElementComponent's creation/removal performance, as every ScrollView and Scrollbar components were subscribing to ElementComponentSystem's global lists.

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

@mvaligursky
Copy link
Contributor

@Maksims - could you please merge main to it and resolve the conflict

@Maksims
Copy link
Collaborator Author

Maksims commented Jan 6, 2025

@Maksims - could you please merge main to it and resolve the conflict

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.

This looks good, but I wonder if we could avoid some code duplication here, which adds some complexity. It could do with some wrapper of some of the code?

@Maksims
Copy link
Collaborator Author

Maksims commented Jan 6, 2025

This looks good, but I wonder if we could avoid some code duplication here, which adds some complexity. It could do with some wrapper of some of the code?

I thought of some more unified way to handle entity setting, subscribe/unsubscribe mechanics also, but every case is so slightly different. This PR reduces overall amount of code in the Engine. I guess some refactoring is possible as a separate PR in the future, but that would require to refactor whole component's code.

@mvaligursky mvaligursky merged commit a2f360d into playcanvas:main_v1 Jan 13, 2025
8 checks passed
mvaligursky pushed a commit that referenced this pull request Jan 13, 2025
* optimize render component rootBone property

* expose rootBone property

* Update src/framework/components/render/component.js

Co-authored-by: Will Eastcott <willeastcott@gmail.com>

* use jsdo import

* .

* ButtonComponent remove EntityReference

* remove reliance on component system global event lists

* remove EntityReference from ScrollbarComponent

* remove EntityReference, optimize ScrollVeiw and Scrollbar

* lint

---------

Co-authored-by: Will Eastcott <willeastcott@gmail.com>
# Conflicts:
#	src/framework/components/scroll-view/component.js
#	src/framework/components/scroll-view/data.js
#	src/framework/components/scrollbar/component.js
#	src/framework/components/scrollbar/data.js
#	test/framework/utils/entity-reference.test.mjs
@mvaligursky
Copy link
Contributor

merged to v2

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.

2 participants