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

Documentation #43

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

agent-q1
Copy link
Contributor

Docs

This Pull Request adds some documentation for the Implementation of HPA* we have in Terasology.

Copy link

@RatMoleRat RatMoleRat left a comment

Choose a reason for hiding this comment

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

There's a few repeated things that should be changed, mostly with formatting (changing capitalization and/or deleting extra lines). I left several more specific comments on specific lines.

src/main/java/org/terasology/navgraph/BaseRegion.java Outdated Show resolved Hide resolved
protected BaseRegion(int id) {
this.id = id;
map = new BitMap();
}

/**
* Marks a point in the region as walkable by characters
* @param x x co-ordinate of the point with respect to region's top-left corner

Choose a reason for hiding this comment

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

Same here (and elsewhere, I won't mark them all) as for the "id id" comment above

public Set<N> getNeighborRegions() {
return neighborRegions;
}

/**
*
* @return The id of the base region along with the number of neighbouring regions

Choose a reason for hiding this comment

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

Start the statement after @return with a lowercase letter

@@ -20,12 +20,17 @@

/**
* @author synopia
* Representation of a any region or floor

Choose a reason for hiding this comment

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

Is the "a" here a typo?

/**
* Creates a new Bitmap
*/

Choose a reason for hiding this comment

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

Get rid of this extra line

@@ -18,7 +18,8 @@
import org.terasology.math.geom.Vector3i;

/**
* @author synopia
* @author synopia Represents a Block through which characters can walk through. They are blocks which have atleast 2

Choose a reason for hiding this comment

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

New line here, too, and space between the two parts of "at least".

@@ -18,7 +18,8 @@
import org.terasology.math.geom.Vector3i;

/**
* @author synopia
* @author synopia Represents a Block through which characters can walk through. They are blocks which have atleast 2
* penetrable blocks above them and are themselves not penetrable

Choose a reason for hiding this comment

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

Not sure why this is indented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That happens as a part of automatic line formatting. I think I'll let it stay

* Finds Walkable Blocks in NavGraphChunk. Blocks are considered Walkable only if there is enough space above them
* to allow for walking
*
* @param map

Choose a reason for hiding this comment

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

What is "map" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added small description

@@ -29,6 +29,13 @@ public WalkableBlockFinder(WorldProvider world) {
this.world = world;
}

/**
* Finds Walkable Blocks in NavGraphChunk. Blocks are considered Walkable only if there is enough space above them

Choose a reason for hiding this comment

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

New line

@@ -72,6 +79,16 @@ private void findNeighbors(NavGraphChunk map) {
}
}

/**
* Connects Cells to adjacent cells if possible

Choose a reason for hiding this comment

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

Could be worded better for more clarity - something like "Connects [each cell? a cell?] to adjacent cells if possible."

Copy link
Member

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

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

Please excuse me obstructing this PR 🙈 If left a bunch of questions that may help to provide JavaDoc that helps somebody who has no clue what all of this is about (like me 😅 ).
I hope to get the message across that the answers to these basic questions are what I'm looking in the documentation of a class.

src/main/java/org/terasology/navgraph/BaseRegion.java Outdated Show resolved Hide resolved
src/main/java/org/terasology/navgraph/BaseRegion.java Outdated Show resolved Hide resolved
protected BaseRegion(int id) {
this.id = id;
map = new BitMap();
}

/**
* Marks a point in the region as walkable by characters
* @param x the x co-ordinate of the point with respect to region's top-left corner
Copy link
Member

Choose a reason for hiding this comment

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

  • what is the "top-left corner" of a region?
  • what defines the sizes of a region?
  • what exactly does "walkable" mean, e.g., is this about solid ground characters can walk on, or about passable blocks characters can move through (might be swimming, crouching, ...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So they way it was created, walkable blocks are all those blocks on which a (1x1x1) cube character can walk. A non penetrable block underneath and some air blocks above. (Currently set to 2 IIRC). The don't include any other features such as swimming/ crouching.

/**
*
* @return the id of the base region along with the number of neighbouring regions
*/
@Override
public String toString() {
return id + "\nrc = " + neighborRegions.size();
Copy link
Member

Choose a reason for hiding this comment

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

If not strictly necessary I'd think that the string representation could simply be something like "NavRegion(<id>, <size>)", avoiding line breaks.

src/main/java/org/terasology/navgraph/BaseRegion.java Outdated Show resolved Hide resolved
src/main/java/org/terasology/navgraph/BitMap.java Outdated Show resolved Hide resolved
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.

3 participants