Skip to content

Add XML doccomments to classes and methods #14

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.

Sign up for GitHub

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

Open
MechWarrior99 wants to merge 6 commits into
base: v2
Choose a base branch
from
Open

Add XML doccomments to classes and methods #14

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2524098
Initial doc-comments on core class's methods
MechWarrior99 Jul 14, 2021
ac730e2
Updated some requested changes.
MechWarrior99 Sep 5, 2021
450774c
Included more implementation details in doc comments
MechWarrior99 Feb 4, 2022
059c366
Added doc-comments to BMesh class and topological methods
MechWarrior99 Feb 4, 2022
e603f73
Minor change to Loop comments
MechWarrior99 Feb 4, 2022
e3d32d5
Reformatted long comments
MechWarrior99 Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 35 additions & 20 deletions Library/Edge.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ namespace BMeshLib
* next one, so the function Next() is provided to return either next1 or
* next2 depending on the vertex of interest.
*/
/// <summary>
/// Links two <see cref="Vertex"/>s together, and may or may not be part of a <see cref="Face"/>.
/// </summary>
/// <remarks>Multiple <see cref="Face"/>s can share the same <see cref="Edge"/>.</remarks>

@eliemichel eliemichel Jul 14, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The technical note above is worth including as well, as a <remarks>

@MechWarrior99 MechWarrior99 Sep 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The technical note above is worth including as well, as a <remarks>

I think it would be better to not to, the end user doesn't really need to know how it works internally. As long as the API is clear how it works that is all the user should need to care about.
Also worth noting that XML comments don't really support much in the way of formatting, so it would be rather unsightly.
Maybe an abridged version would be better to include?

public class Edge
{
public int id; // [attribute]
Expand All @@ -35,28 +39,34 @@ public class Edge
public Edge prev2;
public Loop loop; // first node of the list of faces that use this edge. Navigate list using radial_next

/**
* Tells whether a vertex is one of the extremities of this edge.
*/
/// <summary>
/// Whether the specified <see cref="Vertex"/> is one of the vertices that make up the <see cref="Edge"/>.
/// </summary>
/// <param name="v">The <see cref="Vertex"/> to compare.</param>
/// <returns><c>true</c> if <paramref name="v"/> is used by the <see cref="Edge"/>; otherwise, <c>false</c>.</returns>
public bool ContainsVertex(Vertex v)
{
return v == vert1 || v == vert2;
}

/**
* If one gives a vertex of the edge to this function, it returns the
* other vertex of the edge. Otherwise, the behavior is undefined.
*/
/// <summary>
/// Returns the other <see cref="Vertex"/> that makes up the <see cref="Edge"/> of the specified <see cref="Vertex"/>.
Comment thread
MechWarrior99 marked this conversation as resolved.
Outdated
/// </summary>
/// <remarks>Assumes that the specified <see cref="Vertex"/> is one of the vertices that make up the <see cref="Edge"/>; otherwise, behavior is undefined.</remarks>

@eliemichel eliemichel Jul 14, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

... that make up the <see cref="Edge"/>, i.e. that <see cref="ContainsVertex"/> returns <c>true</c>. Otherwise, **the** behavior is undefined.

@MechWarrior99 MechWarrior99 Sep 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

... that make up the <see cref="Edge"/>, i.e. that <see cref="ContainsVertex"/> returns <c>true</c>. Otherwise, **the** behavior is undefined.

That seems implicitly stated, more so if you read the ContainsVertex comment first. Also I think it is best to avoid referencing the behavior of other methods because they could change or be removed and the comment would then be inaccurate.

/// <param name="v">The <see cref="Vertex"/> to get the other of.</param>
/// <returns>The other <see cref="Vertex"/> that makes up the edge. Shorthand for <c><paramref name="v"/> == <see cref="vert1"/> ? <see cref="vert2"/> : <see cref="vert1"/></c>.</returns>
Comment thread
MechWarrior99 marked this conversation as resolved.
Outdated
public Vertex OtherVertex(Vertex v)
{
Debug.Assert(ContainsVertex(v));
return v == vert1 ? vert2 : vert1;
}

/**
* If one gives a vertex of the edge to this function, it returns the
* next edge in the linked list of edges that use this vertex.
*/
/// <summary>
Comment thread
MechWarrior99 marked this conversation as resolved.
/// Returns the next <see cref="Edge"/> in the linked list of edges that use the specified <see cref="Vertex"/>.
/// </summary>
/// <remarks>Assumes that the specified <see cref="Vertex"/> is one of the vertices that make up the <see cref="Edge"/>; otherwise, behavior is undefined.</remarks>
/// <param name="v"></param>
/// <returns>The next <see cref="Edge"/> in the linked list of edges that use <paramref name="v"/>. Shorthand for <c><paramref name="v"/> == <see cref="vert1"/> ? <see cref="next1"/> : <see cref="next2"/></c>.</returns>

@eliemichel eliemichel Jul 14, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

NB: The return edge and this edge have the vertex v in common.

public Edge Next(Vertex v)
{
Debug.Assert(ContainsVertex(v));
Expand All @@ -73,9 +83,12 @@ public void SetNext(Vertex v, Edge other)
else next2 = other;
}

/**
* Similar to Next() but to go backward in the double-linked list
*/
/// <summary>
/// Returns the previous <see cref="Edge"/> in the linked list of edges that use the specified <see cref="Vertex"/>.
Comment thread
MechWarrior99 marked this conversation as resolved.
Outdated
/// </summary>
/// <remarks>Assumes that the specified <see cref="Vertex"/> is one of the vertices that make up the <see cref="Edge"/>; otherwise, behavior is undefined.</remarks>
/// <param name="v"></param>
/// <returns>The previous <see cref="Edge"/> in the linked list of edges that use <paramref name="v"/>. Shorthand for <c><paramref name="v"/> == <see cref="vert1"/> ? <see cref="prev1"/> : <see cref="prev2"/></c>.</returns>
public Edge Prev(Vertex v)
{
Debug.Assert(ContainsVertex(v));
Expand All @@ -92,9 +105,10 @@ public void SetPrev(Vertex v, Edge other)
else prev2 = other;
}

/**
* Return all faces that use this edge as a side.
*/
/// <summary>
/// Returns all <see cref="Face"/>s that use the <see cref="Edge"/> as a side.
/// </summary>
/// <returns>all <see cref="Face"/>s that use the <see cref="Edge"/> as one of it's sides.</returns>
Comment thread
MechWarrior99 marked this conversation as resolved.
Outdated
public List<Face> NeighborFaces()
{
var faces = new List<Face>();
Expand All @@ -110,9 +124,10 @@ public List<Face> NeighborFaces()
return faces;
}

/**
* Compute the barycenter of the edge's vertices
*/
/// <summary>
/// The center of the <see cref="Edge"/>'s vertices.
/// </summary>
/// <returns>The center between <see cref="vert1"/> and <see cref="vert2"/>.</returns>
public Vector3 Center()
{
return (vert1.point + vert2.point) * 0.5f;
Expand Down
38 changes: 23 additions & 15 deletions Library/Face.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,20 @@ namespace BMeshLib
* makes sense only 1. for clarity, because loops are a less intuitive
* object and 2. to store face attributes.
*/
/// <summary>
/// Represents a face of a mesh in a <see cref="BMesh"/>.

@eliemichel eliemichel Jul 14, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Again I think it is worth including the technical comment above as a remark. I feel it is important that users are well aware of the specifics of the data structure they are using.

@MechWarrior99 MechWarrior99 Sep 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Again I think it is worth including the technical comment above as a remark. I feel it is important that users are well aware of the specifics of the data structure they are using.

I disagree again on this one, but more so. The comment almost exclusively only explains and justifies the reason for the implementation. This information is not needed for an end user imo. It could also give the impression that the Face doesn't really need to be used.

/// </summary>
public class Face
{
public int id; // [attribute]
public Dictionary<string, AttributeValue> attributes; // [attribute] (extra attributes)
public int vertcount; // stored for commodity, can be recomputed easily
public Loop loop; // navigate list using next

/**
* Get the list of vertices used by the face, ordered.
*/
/// <summary>
/// Returns the ordered <see cref="Vertex"/>s that are used by the <see cref="Face"/>.
Comment thread
MechWarrior99 marked this conversation as resolved.
Outdated
/// </summary>
/// <returns>The vertices that make up the corner's of the <see cref="Face"/>.</returns>
public List<Vertex> NeighborVertices()
{
var verts = new List<Vertex>();
Expand All @@ -34,10 +38,13 @@ public List<Vertex> NeighborVertices()
return verts;
}

/**
* Assuming the vertex is part of the face, return the loop such that
* loop.vert = v. Return null otherwise.
*/
/// <summary>
/// Returns the <see cref="BMeshLib.Loop"/> in the <see cref="Face"/> whose <see cref="Loop.vert"/> matches the specified <see cref="Vertex"/>.
Comment thread
MechWarrior99 marked this conversation as resolved.
Outdated
/// </summary>
/// <param name="v">The <see cref="Vertex"/> to get the <see cref="BMeshLib.Loop"/> of.</param>
/// <returns>
/// The <see cref="BMeshLib.Loop"/> of the <see cref="Face"/> whose <see cref="Loop.vert"/> matches <paramref name="v"/> if it is part of the <see cref="Face"/>; otherwise, <c>null</c>.
/// </returns>
public Loop Loop(Vertex v)
{
if (this.loop != null)
Expand All @@ -53,11 +60,11 @@ public Loop Loop(Vertex v)
return null;
}

/**
* Get the list of edges around the face.
* It is garrantied to match the order of NeighborVertices(), so that
* edge[0] = vert[0]-->vert[1], edge[1] = vert[1]-->vert[2], etc.
*/
/// <summary>
/// Returns the ordered <see cref="Edge"/>s around the <see cref="Face"/>.
/// </summary>
/// <remarks>Garrantied to match the order of <see cref="NeighborVertices"/>. So that <c>edge[0] = vert[0]-->vert[1], edge[1] = vert[1]-->vert[2], etc.</c></remarks>
/// <returns>The <see cref="Edge"/>s that make up the <see cref="Face"/>.</returns>
public List<Edge> NeighborEdges()
{
var edges = new List<Edge>();
Expand All @@ -73,9 +80,10 @@ public List<Edge> NeighborEdges()
return edges;
}

/**
* Compute the barycenter of the face vertices
*/
/// <summary>
/// The center of the vertices that are used by the <see cref="Face"/>.
/// </summary>
/// <returns>The center of <see cref="Face"/>.</returns>
public Vector3 Center()
{
Vector3 p = Vector3.zero;
Expand Down
19 changes: 11 additions & 8 deletions Library/Loop.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ namespace BMeshLib
* namely the radial list, that enables iterating over all the faces using
* the same edge.
*/
/// <summary>
/// Represents a portion of a <see cref="Face"/>.

@eliemichel eliemichel Jul 14, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

All above comment again :) What is a bit misleading is that a Loop object can be seen either as representing the whole loop or one node (i.e. element) of this loop. It's a bit like with a linked list:

class LinkedList {
    object head;
    LinkedList tail;
}

Here a LinkedList technically only contains the data of the head, so only one of the nodes of the list, but it can be thought as the whole list as well. It's more subtle with the Loop because first it's a linked list with the end connected to the beginning, so that there is no item flagged as "end" and "beginning" actually, any item can represent the whole loop; and secondly it is also used as an element/node of another looped list, which contain all the edges which share a same vertex.

Maybe the most intuitive way to describe a loop item is to tell it is the combination of a vertex, an edge and a face, and that it is meant to provide fast access to neighboring edges, either by "turning around" the face (Next/Prev) or "turning around" the vertex (RadialNext/RadialPrev).

(BTW I realized lately that a face can be made of several sequences of loop actually, say if it contains a hole in the middle, but this is an edge case we don't support here)

@MechWarrior99 MechWarrior99 Sep 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I feel like the technical comment doesn't actually clear anything up, at least for me. Though I also don't like the doc comment I write since it doesn't really tell anything useful about the class itself.

While writing the doc comments, I am trying to get away from telling the user how the underlining structure works, and instead tell them how it works in the context of the rest of the API. While my doc comment for the Loop isn't great, I feel like it at least illustrates what I mean. It tells you what the Loop is in relation to the rest of the API.

Maybe the most intuitive way to describe a loop item is to tell it is the combination of a vertex, an edge...

I think that this is the most understandable way I have seen to explain a Loop.

@MechWarrior99 MechWarrior99 Sep 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"turning around" the vertex (RadialNext/RadialPrev).

What are you talking about here?

@eliemichel eliemichel Sep 6, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

image

@eliemichel eliemichel Sep 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If you don't understand the technical comment don't include it to <summary> but please don't remove it it is at least useful for myself to remember!

@MechWarrior99 MechWarrior99 Sep 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I need to look at it more, but I think that diagram helps. Of course! I didn't remove any of the technical comments on the classes, I think they are important when working with the source code to describe how and why things are structured!
I just don't think that info is needed/should be given when working with the API (Apposed to reading/modifying the source code)

/// </summary>
public class Loop
{
public Dictionary<string, AttributeValue> attributes; // [attribute] (extra attributes)
Expand All @@ -38,10 +41,10 @@ public Loop(Vertex v, Edge e, Face f)
SetFace(f);
}

/**
* Insert the loop in the linked list of the face.
* (Used in constructor)
*/
/// <summary>
/// Insert the <see cref="Loop"/> in to the linked list of the specified <see cref="Face"/>.

@eliemichel eliemichel Jul 14, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The note "Used in constructor" meant that this was likely not supposed to be used by external code, but exposed publicly anyway in case people would like to do weird things.

@MechWarrior99 MechWarrior99 Sep 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kind of feel like it just shouldn't be public then. If there is something that it enables the user to do, then maybe it should have a proper API, but otherwise I think it should be private since I don't think we should let the user explicitly break things like this.

/// </summary>
/// <param name="f">The <see cref="Face"/> to insert the <see cref="Loop"/> in to.</param>
public void SetFace(Face f)
{
Debug.Assert(this.face == null);
Expand All @@ -63,10 +66,10 @@ public void SetFace(Face f)
this.face = f;
}

/**
* Insert the loop in the radial linked list.
* (Used in constructor)
*/
/// <summary>
/// Insert the <see cref="Loop"/> in to the radial linked list.
/// </summary>
/// <param name="e">The <see cref="Edge"/> to insert the <see cref="Loop"/> in to.</param>
public void SetEdge(Edge e)
{
Debug.Assert(this.edge == null);
Expand Down
20 changes: 14 additions & 6 deletions Library/Vertex.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ namespace BMeshLib
* The vertex position does not affect topological algorithms, but is used by
* commodity functions that help finding the center of an edge or a face.
*/
/// <summary>
/// Corresponds to a point in space, and is used by <see cref="Edge"/> and <see cref="Face"/> to construct a <see cref="BMesh"/>.
/// </summary>
/// <remarks>
/// Multiple <see cref="Edge"/>s and <see cref="Face"/>s can use the same <see cref="Vertex"/>, and multiple vertices can be located at the exact same position.
/// </remarks>

@eliemichel eliemichel Jul 14, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Add the other remark about data structure details.

@MechWarrior99 MechWarrior99 Sep 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And what I said about the Loop comment goes for here too. I feel like some of the info in the technical comment could be added, but I don't think that it should all be added.

@eliemichel eliemichel Sep 6, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

So same answer ^^ Put it in a remark or even in an invented <dev> téag but still include the whole of it because I need it to remember.

public class Vertex
{
public int id; // [attribute]
Expand All @@ -35,9 +41,10 @@ public Vertex(Vector3 _point)
point = _point;
}

/**
* List all edges reaching this vertex.
*/
/// <summary>
/// Returns all <see cref="Edge"/>s that reach the <see cref="Vertex"/>.
/// </summary>
/// <returns>All <see cref="Edge"/>s that reach the <see cref="Vertex"/>. Uses <see cref="Edge.Next(Vertex)"/> from <see cref="edge"/> until it reaches <see cref="edge"/> again.</returns>
public List<Edge> NeighborEdges()
{
var edges = new List<Edge>();
Expand All @@ -53,9 +60,10 @@ public List<Edge> NeighborEdges()
return edges;
}

/**
* Return all faces that use this vertex as a corner.
*/
/// <summary>
/// Returns all <see cref="Face"/>s that use the <see cref="Vertex"/> as a corner.
/// </summary>
/// <returns>All <see cref="Face"/>s that use the <see cref="Vertex"/> as one of it's corners.</returns>
public List<Face> NeighborFaces()
{
var faces = new HashSet<Face>();
Expand Down