479 onboarding of new see members yvo #485
Conversation
Added functionality for GameNodeMarker, MarkAction and MarkNetAction. Extended ActionStateType with a new spacebar action for marking an element.
…ew-see-members-yvo
Changed documentation as suggested Co-authored-by: Falko <10247603+falko17@users.noreply.github.com>
Co-authored-by: Falko <10247603+falko17@users.noreply.github.com>
| /// <summary> | ||
| /// The position of the new marker in world space. | ||
| /// </summary> | ||
| public readonly Vector3 Position; |
There was a problem hiding this comment.
GameNodeMarker could take care of position and scale of the sphere. No need to bother with these here.
| /// The game object that was added when this action was executed. It is saved so | ||
| /// that it can be removed by Undo(). | ||
| /// </summary> | ||
| private GameObject addedSphere; |
There was a problem hiding this comment.
The unmarking, i.e., destruction of the added sphere, could be handled by GameNodeMarker.
| { | ||
| GameObject sphere; | ||
|
|
||
| if(parent == null) |
There was a problem hiding this comment.
There should be a blank between if and (.
| base.Undo(); | ||
| if (addedSphere != null) | ||
| { | ||
| new DeleteNetAction(addedSphere.name).Execute(); |
There was a problem hiding this comment.
addedSphere.name would need to be unique. In your implementation I cannot see how this is guaranteed.
Moreover, DeleteNetAction is generally intended to be used to delete nodes or edges. We would need to check whether it is safe to be used for other kinds of game objects, too.
| return new HashSet<string> | ||
| { | ||
| memento.Parent.name, | ||
| memento.NodeID |
There was a problem hiding this comment.
The conflict resolution via GetChangedObjects() is intended for nodes and edges, not for decorations of these. Since marks can be toggled only via their containing node, the conflict resolution for its containing node should suffice. Moreover, memento.NodeID would need to be unique.
| /// <param name="worldSpaceScale">the scale in world space of the new marker</param> | ||
| /// <returns>new marker game object or null if none could be created or a marker already exists</returns> | ||
| /// <exception cref="ArgumentNullException">If <paramref name="parent"/> is <c>null</c>.</exception> | ||
| public static GameObject AddSphere(GameObject parent, Vector3 position, Vector3 worldSpaceScale) |
There was a problem hiding this comment.
As stated above, AddSphere could determine the necessary position and scale of the sphere. No need to pass these as parameters.
| sphere = GameObject.CreatePrimitive(PrimitiveType.Sphere); | ||
| float diameter = Math.Min(worldSpaceScale.x, worldSpaceScale.y); | ||
| sphere.transform.localScale = new Vector3(diameter, diameter, diameter); | ||
| sphere.transform.position = new Vector3(position.x, position.y + 0.1f, position.z); |
There was a problem hiding this comment.
It would be good to use a named constant instead of the magic number 0.1f.
| if (child.name == "Sphere") | ||
| { | ||
| //FIXME: network operation for deleting an existing marker not working properly | ||
| //new DeleteNetAction(sphere.name).Execute(); |
There was a problem hiding this comment.
DeleteNetAction should never be called here. That needs to be handled by the client of GameNodeMarker. This helper class GameNodeMarker whole purpose is to make changes to the scene.
marking a node with a red sphere above