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 new Zones appearing as parent of a ghost child #874

Merged
merged 6 commits into from
Nov 28, 2020
Merged

Fix new Zones appearing as parent of a ghost child #874

merged 6 commits into from
Nov 28, 2020

Conversation

AleziaKurdis
Copy link
Contributor

Fix for Issue #869

This excludes the local and avatar entities from what is returns by Entities.getChildrenIDs
to avoid the selection tools of the Zone entities to be considered as Children of it.
This was necessary for the display of the hierarchy status, but also for the "Add Children to Selection".

For the QA:
1- Open the "Create" Application
2- Make sure to have the "Hierarchy" column displayed in the entity list.
3- Create a new Zone entity
Before this fix:
The new zone entity was displayed as being a Top Parent. (because there was a local entity parented to it. (to display the grid box)
In In-world view, the bounding box was also indicating that it was a Top Parent (it was displayed in Orange instead of light gray)
If you selected that new zone, and do "Selection... > Add Children to Selection", the bounding box turned light gray as a multiple selection. If you were keeping CTRL pressed and deselect the zone only, then you could see the invisible local entity selected with a bounding box in cyan (as a child).

After this fix:
No more trace of any children, since local entities are now ignored when we get the list of the children.
So new zones are no more displayed as a Parent of a child that we didn't expect. And the "Selection... > Add Children to Selection" won't consider this false child too.

This excludes the local and avatar entities from what is returns by Entities.getChildrenIDs
to avoid the selection tools of the Zone entities to be considered as Children of it.
This was necessary for the display of the hierarchy status, but also for the "Add Children to Selection".
This excludes the local and avatar entities from what is returns by Entities.getChildrenIDs
to avoid the selection tools of the Zone entities to be considered as Children of it.
This was necessary for the display of the hierarchy status, but also for the "Add Children to Selection".
@HifiExperiments HifiExperiments added allow-build-upload Allows the upload of a build for non white-listed users rebuild rebuild through the GithubActions and removed rebuild rebuild through the GithubActions labels Nov 19, 2020
@ctrlaltdavid ctrlaltdavid added CR Approved At least one code reviewer has approved the PR. needs testing (QA) The PR is ready for testing labels Nov 19, 2020
@two-one-five
Copy link
Contributor

According to QA it worked as expected: I created the zone entity and it doesn't show any children under it on the icon. Though, I didn't test any parenting as I was unsure what causes what in that respect.

@two-one-five two-one-five added this to In progress in 2020.3.2 Demeter Release via automation Nov 19, 2020
@two-one-five two-one-five added this to the 2020.3.2 Release milestone Nov 19, 2020
@two-one-five two-one-five added QA Approved The PR has been tested successfully. bugfix and removed needs testing (QA) The PR is ready for testing labels Nov 19, 2020
@FluffyJenkins
Copy link
Contributor

@AleziaKurdis May I suggest making a tickbox to allow avatar/local entities to show up as children? (can be off by default which would equate to the functionality you are suggesting), Means that people who are creating complex avatars for example.

@AleziaKurdis
Copy link
Contributor Author

Hmm... Can we use the create application to edit avatar entities?

Because we can't for "Local" entities. They are not visible in the list and can't be selected.
To be able to create a local entity or set it a local entities as child of another entity, the only way is to do it using a script.

@two-one-five
Copy link
Contributor

We are able to adjust wearables with the Create App as far as I know.

@AleziaKurdis
Copy link
Contributor Author

We will put this PR on HOLD.
I will change the strategy.
and figure to exclude those specific "tools" entities
instead of excluding local entities (and avatar entities)
Seems that eventually, Local entities could be visible like all the others.

@two-one-five two-one-five added do not merge do not merge due to issues or pending updates postponed This will be revisited at a later time. and removed QA Approved The PR has been tested successfully. labels Nov 20, 2020
@two-one-five two-one-five added this to In progress in 2020.3.3 Demeter Release via automation Nov 20, 2020
@two-one-five two-one-five removed this from In progress in 2020.3.2 Demeter Release Nov 20, 2020
Excluding EntityShapeVisualizer from Children list
Now a name is given to the EntityShape of the EntityShapeVisualizer (used to display the zone)
so we can now exclude them from the children list when it's time to figure if there are children for an entity.
(This is a better approach than excluding al the local entities)
The name has been used because it gives better results than trying to use the id map that arrive always too late.
The name is unique for a session, it includes a UUID.
Excluding EntityShapeVisualizer from Children list
Now a name is given to the EntityShape of the EntityShapeVisualizer (used to display the zone)
so we can now exclude them from the children list when it's time to figure if there are children for an entity.
(This is a better approach than excluding al the local entities)
The name has been used because it gives better results than trying to use the id map that arrive always too late.
The name is unique for a session, it includes a UUID.
@AleziaKurdis
Copy link
Contributor Author

Finally, I change the approach.
I exclude now only the Entity Shape Visualizer
instead of systematically all the local entities (and the avatar entities by the same time)

So this will continue to work for all types of entities. (domain, local, avatar)

I still don't think it will be viable to have the local entities visible in the create Application
it might seriously complexify everything to be able to protect the selection tool to self-edit

If we do this, I suggest that we implement a property to have specific local entities to never be returned by the findEntities### methods, getChildrens, and certainly some others.

@AleziaKurdis
Copy link
Contributor Author

You can now remove the "Do not Merge" flag and gives this to QA.

@two-one-five two-one-five added needs CR (code review) needs testing (QA) The PR is ready for testing and removed CR Approved At least one code reviewer has approved the PR. do not merge do not merge due to issues or pending updates postponed This will be revisited at a later time. labels Nov 21, 2020
scripts/system/create/edit.js Outdated Show resolved Hide resolved
2020.3.3 Demeter Release automation moved this from In progress to Review in progress Nov 22, 2020
Minor code adjustments
2020.3.3 Demeter Release automation moved this from Review in progress to Reviewer approved Nov 22, 2020
@ctrlaltdavid ctrlaltdavid added CR Approved At least one code reviewer has approved the PR. and removed needs CR (code review) labels Nov 22, 2020
@Aitolda
Copy link
Collaborator

Aitolda commented Nov 24, 2020

Seems to be working as expected now.

@two-one-five two-one-five added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Nov 25, 2020
Copy link
Contributor

@two-one-five two-one-five left a comment

Choose a reason for hiding this comment

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

Works fine for me as well.

@two-one-five two-one-five added this to In progress in 2020.3.2 Demeter Release via automation Nov 25, 2020
@two-one-five two-one-five removed this from Reviewer approved in 2020.3.3 Demeter Release Nov 25, 2020
@ctrlaltdavid ctrlaltdavid changed the title Fix for Issue #869 Fix new Zones appearing as parent of a ghost child Nov 25, 2020
@two-one-five two-one-five merged commit 60e59ab into vircadia:master Nov 28, 2020
2020.3.2 Demeter Release automation moved this from In progress to Done Nov 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
allow-build-upload Allows the upload of a build for non white-listed users bugfix CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully. rebuild rebuild through the GithubActions
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

New Zones appear as parent of a ghost child. Issue with Entities.getChildrenIDs()
7 participants