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

642 face cam #646

Merged
merged 35 commits into from
Oct 8, 2023
Merged

642 face cam #646

merged 35 commits into from
Oct 8, 2023

Conversation

koschke
Copy link
Collaborator

@koschke koschke commented Oct 5, 2023

This branch is workings towards showing the video of all desktop player on all clients. Currently that is not the case as reported in issue #642. The problem is not yet fixed but in the course of starting to work on this issue here in this branch I fixed a null-pointer dereference during start-up of a player's avatar, reported by @falko17.

The changes made in this pull request fixes the problem and should be merged into the master. The branch will continue to exist for the additional work required for issue #642.

Previously, it was set in the prefab, but we changed its visibility
from public to private, in which case it is not serialized by default.
The FaceCam prefab will become a child of a player prefab, which is
also a NetworkObject. Nested NetworkObjects are not allowed.
The FaceCam prefab will become a child of a player prefab, which is
also a NetworkObject. Nested NetworkObjects are not allowed.
That is why we just instantiate, but not spawn it.
It is used in the context of players.
Is does not need to be instantiated here. We want it to be
instantiated for all players, not only those for the player on the
server.
Previously, it was only deactivated, but it still didn't work.
when OnNetworkSpawn(). OnNetworkSpawn() can be called before Start().
instead of adding them as an instantiated child. This way
we can make changes to the FaceCam only once for all players.
It caused trouble even though it was deactivated.
FaceCamOnOffToggle() is using this field, too.
LocalPlayer is now in a class of its own. It didn't belong here.
@koschke koschke requested a review from falko17 October 5, 2023 09:07
@koschke koschke added the bug Something isn't working label Oct 5, 2023
@koschke koschke added this to the Qt World Summit milestone Oct 5, 2023
This may be the case on artificial nodes,
such as the artificial roots on
reflexion cities.
Copy link
Collaborator

@falko17 falko17 left a comment

Choose a reason for hiding this comment

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

A few minor comments.

Assets/SEE/Game/LocalPlayer.cs Outdated Show resolved Hide resolved
Assets/SEE/Game/Worlds/PlayerSpawner.cs Outdated Show resolved Hide resolved
Assets/SEE/Game/Worlds/PlayerSpawner.cs Outdated Show resolved Hide resolved
Assets/SEE/Tools/FaceCam/FaceCam.cs Show resolved Hide resolved
Assets/SEE/Tools/FaceCam/FaceCam.cs Outdated Show resolved Hide resolved
Assets/SEE/Tools/FaceCam/FaceCam.cs Show resolved Hide resolved
Copy link
Collaborator Author

@koschke koschke left a comment

Choose a reason for hiding this comment

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

There are two violations of our naming rules.

koschke and others added 5 commits October 5, 2023 14:50
Copy link
Collaborator

@falko17 falko17 left a comment

Choose a reason for hiding this comment

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

Should be good to merge once the pipeline runs through.

@koschke koschke enabled auto-merge October 6, 2023 12:40
@koschke koschke merged commit 64b26a2 into master Oct 8, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants