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

[Pointcloud] New interfaces and automatic asset reloading #49

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

Fireronin
Copy link
Contributor

Resolves #40

Adds AcquirePointcloudFromAsset and UpdatePointCloud
Interfaces to feature processor.

First one creates a ponint cloud based on asset and keeps track of it's changes.
Second one allows for partial updates to pointcloud data

@Fireronin Fireronin changed the title Added new interfaces and automatic asset reloading [Pointcloud] New interfaces and automatic asset reloading Aug 7, 2024
@Fireronin
Copy link
Contributor Author

I'm unable to fix those formatting issues like I mash reformat and nothing happens, I think my formatting style is correct as I was able to fix other issues using the same formatting

Copy link
Collaborator

@michalpelka michalpelka left a comment

Choose a reason for hiding this comment

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

Please apply clang-format-15

@michalpelka michalpelka force-pushed the kr/Pointcloud-update-from-main branch 2 times, most recently from c7227a7 to 82d0098 Compare August 9, 2024 11:34
@michalpelka
Copy link
Collaborator

I've fixed code with help of o3de Discord : https://discord.com/channels/805939474655346758/816043601754325023/1271239320698748951
Now hot-realoding works as expected.
Our error was, that we have old and new asset. We should keep reference to AZ::Asset<AZ::Data> and we are responsible to swap this reference. Without that we are stuck with reference to old data. When reference count on old asset goes to zero, it is destroyed. It is smart, but requires understanding.

untitled.webm

With permission from @Fireronin, I've forced pushed code.

@michalpelka
Copy link
Collaborator

I've added @norbertprokopiuk @zakmat to review, since they are using this code.

Co-authored-by: Krzysztof Rymski <krzysztof.rymski@robotec.ai>

Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
Copy link
Member

@zakmat zakmat left a comment

Choose a reason for hiding this comment

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

This is a great improvement. Thanks! I would love to see such handling in other places where we BlockUntilCompleted

There is a small but significant bug in the component's Activate - no check for visibility.
Additionally, the m_numPoints is no longer updated.

@Fireronin Fireronin requested a review from zakmat August 13, 2024 11:41
Copy link
Member

@zakmat zakmat left a comment

Choose a reason for hiding this comment

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

Looks good!

@Fireronin Fireronin merged commit db16a34 into main Aug 13, 2024
1 check passed
@patrykantosz patrykantosz deleted the kr/Pointcloud-update-from-main branch September 30, 2024 11:22
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.

[Pointcloud] Eliminate blocking loading of pointcloud assets
3 participants