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

Implement performance metrics BehaveTree Node #103

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

goudcode
Copy link
Contributor

Description

I've implemented the performance per tree in the Godot engine profiler.
Time is being showed in nanosecconds and tree will be automatically added and removed to the profiler.

It might be better to disable all the metric code when running in release mode, example: use if OS.is_debug_build()
Any ideas are welcome.

Addressed issues

#89

Screenshots

afbeelding

@bitbrain bitbrain added the 🍯 feature A new addition to this addon. label Jan 22, 2023
@goudcode goudcode marked this pull request as draft January 22, 2023 22:27
Copy link
Owner

@bitbrain bitbrain left a comment

Choose a reason for hiding this comment

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

Very good start! Apart from a few smaller comments this looks generally good.

Could we also add additional metrics like described on #89?

Metric Name Description
beehave/node_count The number of total beehave nodes in the scene
beehave/active_count Number of active behaviour trees in the scene

Also reminder, that there is no need to check for debug mode, as Godot will only query the monitors inside the editor anyways and ensures not to call these in production mode.

@@ -22,6 +22,8 @@ enum {

var actor : Node

var _tree_metric_name : String
Copy link
Owner

Choose a reason for hiding this comment

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

we may want to give this a better name: as it is tracking the process_time of this tree, perhaps something like this:

Suggested change
var _tree_metric_name : String
var _process_time_metric_name : String

(we are inside the tree already so using "tree" is redundant)

func _enter_tree() -> void:
# Get the name of the parent node name for metric
var parent_name = get_parent().name
_tree_metric_name = "beehave/process_time/%s(%s)" % [parent_name, get_instance_id()]
Copy link
Owner

Choose a reason for hiding this comment

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

From a structural perspective, we may want to have multiple metrics as part of the same tree. Therefore, I'd group them differently:

Suggested change
_tree_metric_name = "beehave/process_time/%s(%s)" % [parent_name, get_instance_id()]
_tree_metric_name = "beehave/%s-%s/process_time" % [parent_name, get_instance_id()]


func _enter_tree() -> void:
# Get the name of the parent node name for metric
var parent_name = get_parent().name
Copy link
Owner

Choose a reason for hiding this comment

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

Using get_parent() here is wrong, as there are different ways to associate a node to a tree (see actor_node_path export property)

Suggested change
var parent_name = get_parent().name
var parent_name = actor.name

@bitbrain bitbrain added the 🟡 changes requested Changes have been requested from the contributor. label Jan 23, 2023


# Called by the engine to profile this tree
func _get_tree_metric() -> float:
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, this method also should be renamed to something like this:

Suggested change
func _get_tree_metric() -> float:
func _get_process_time_metric_value() -> float:

@goudcode
Copy link
Contributor Author

The metric is now registered in the ready() function instead of the _enter_tree().
The reason for this is that the actor was not known in that function.

- Refactored variable names to tell what the metric is.
- Parent name is now obtained by actor property
@goudcode goudcode marked this pull request as ready for review January 25, 2023 17:22
@bitbrain bitbrain removed the 🟡 changes requested Changes have been requested from the contributor. label Jan 26, 2023
Copy link
Owner

@bitbrain bitbrain left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@bitbrain bitbrain merged commit d9995d7 into bitbrain:godot-4.x Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍯 feature A new addition to this addon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants