(Visualisation) Example thoughts

CitrusWire commented 3 years ago

This looks super promising! A few initial thoughts regarding the example. Just suggestions, all meant constructively:

You may want to make the example project an "obvious" example and make it into a project.godot file so it can be imported as a stand alone project.
The example contains the default comments; these should be removed as they're not pertinent:
```
# Declare member variables here. Examples:
# var a = 2
# var b = "text"
```
Add more comments to the GDScript code (especially gridmap.gd). I know that most devs hate writing docs/comments, but they're super useful for learning stuff for many people. It's always best to assume the user knows far less about the subject matter than you do, and they also may not be as au-fait as you are with GDScript. See also: Me. ;-)
Probably don't want Godot warning blockers in the example: # warning-ignore:unused_class_variable

But yes, great job, looking forward to seeing where this goes

astrale-sharp commented 3 years ago

Thanks for your thoughts :3

good call about


# Declare member variables here. Examples:
# var a = 2
# var b = "text"

(I'll deal with that ;) )

the demo you are referring too isn't exactly an example, its goal is only to demonstrate what the API can do, we have two heavily commented example in res://Project_Example/ maybe the README.md wasn't clear ^^'

what do you think about theses examples?

CitrusWire commented 3 years ago

res://Project_Example/

Hmm, there is no res://Project_Example/ or anything similar to it in my download. No mention of them in DOCUMENTATION.md either (the only markdown file in the thing I downloaded....)

Ah... I downloaded from git directly: https://github.com/MatejSloboda/Dijkstra_map_for_Godot/archive/master.zip - that only includes the addons directory. I see the Godot asset library as the rest of the repo (including the .github dir; probably don't want that in there?)

CitrusWire commented 3 years ago

The project examples are much much better! :-) Really helpful!

Suggestions / thoughts:

General thoughts

Something at the top of the main .gd files in the projects explaining what the project demonstrates (that archers will go to a distance).
Any instructions for the user as well (you can click to move the dragon for instance).
Some constants for the user to tweak could be helpful from a learning perspective. Movement costs for example. In fact as a general rule it's probably an idea to stick all the (effectively) constants at the top.

Specific thoughts

project_example

I love this level of commenting! :-)

A note in there explaining why only doing one of the two maps (It's an optimisation as they both start the same).

    #we also need to specify a terrain type for the tile.
    #terrain types can then have different weights, when DijkstraMap is recalculated
    var terrain_type = self.get_cellv(pos)
    dijkstra_map_for_archers.add_point(id, terrain_type)

This is commented out:

        #we skip adding connection if point doesnt exist
#           if id_of_neighbour == -1:
#               continue

As are a few other lines, but it's not clear why.

Not sure this wants to be in there (there's a few of them): assert(res == 0) and # breakpoint

Creates the following two variables but never uses them:

var cost_map = dijkstra_map_for_archers.get_cost_map()
var direction_map = dijkstra_map_for_archers.get_direction_map()

the indented direction should be "intended"
Not sure if this is a bug in the pathfinding or the game project, but the chasing units struggle to get around corners. I suspect they're trying to go diagonal but they have to go in orthogonal directions instead.

turn_based.gd

#first argument is the rectangle. - what's the rectangle for?
since will will specify terrain later - "we will"
This one is less standalone. There's more detail in the project_example version. For example about the id's/pos dictionaries and how they relate to each other.

#now highlight the tiles
#first we get all tiles with cost below "max_cost"
var point_ids = Array(dijkstra_map.get_all_points_with_cost_between(0.0, max_cost))

A line or two explaining why you're doing it this way rather than setting a lower max_cost to calculate (as was done with the archers) and then highlighting all resultant squares.

=====

Again, great work, I'm really looking forward to playing with this.

MatejSloboda / Dijkstra_map_for_Godot