Update documentation for v2.0

Question

Update documentation for v2.0

Closed this issue a year ago · 11 comments

Answer 1 · 2023-02-27T00:35:45.000Z

Any progress on this?

Answer 2 · 2023-03-02T04:53:07.000Z

I took a little break from this project, but I'll be back in about a week or so to tackle the remaining issues. I made changes to the documentation comments as I rewrote things. so, ignoring minor errors, they should already be up to date in the latest commit. I believe the only things left to do are to update the project's Read-Me and "Getting Started" docs.

Answer 3 · 2023-03-02T18:55:51.000Z

All good. I was mostly referring to the "Getting Started" docs, as currently trying to comprehend the State module with only included documentation is a tall order.

Aside from updating outdated example code, perhaps it needs a little work to add clarification on certain things (ex. why are some class factories separate while others are internal?), or slightly better naming (ex. FrayStateNodeStateMachine and related).

Answer 4 · 2023-03-04T02:51:22.000Z

I was playing with this as a test, but figure out this would be a good introduction.

extends Node3D

# Create a sequencelist and a matcher objetcs.
@onready var sequence_list := FraySequenceList.new()
@onready var sequence_matcher := FraySequenceMatcher.new()

# Called when the node enters the scene tree for the first time.
func _ready() -> void:
#	Register Godot inputs to frayinput. I added one and name it shoot.
	FrayInputMap.add_bind_action("right", "ui_right")
	FrayInputMap.add_bind_action("left", "ui_left")
	FrayInputMap.add_bind_action("up", "ui_up")
	FrayInputMap.add_bind_action("down", "ui_down")
	FrayInputMap.add_bind_action("shoot", "shoot")
	
#	Composite direction like down_left can be registered in a loop
	for Y in ["down","up"]:
		for X in ["left","right"]:
			FrayInputMap.add_composite_input(Y + '_' + X,\
				FrayCombinationInput.builder()\
					.add_component(FraySimpleInput.from_bind(Y))\
					.add_component(FraySimpleInput.from_bind(X))\
					.is_virtual().build()
				)
	
#	Create sequences
#	for each direction, register a sequence when the direction is double tapped, like a dash
	for DIR in ["down","up","left","right","down_left","down_right","up_left","up_right"]:
		sequence_list.add("dash_"+DIR, FraySequencePath.from_first(DIR).then(DIR))
#	Simple charge move
	sequence_list.add("Test_chargeshot", FraySequencePath.from_first("left",1).then("right").then("shoot"))
#	Another example 
	sequence_list.add("Test_shoryuken", FraySequencePath.from_first("down").then("right").then("down_right").then('shoot'))
	
#	TODO : Switch the side of the chargeshot and shoryuken
	
#	Register the sequence list to the matcher
	sequence_matcher.initialize(sequence_list)
	
#	Send any inputs event to be read the sequence matcher
	FrayInput.input_detected.connect(sequence_matcher.read)
#	In case of a match event, print the name of the sequence. I use a callable here.
	sequence_matcher.match_found.connect(func(seq:StringName):
		print(seq)
		)
	
#	Very nice feature, print the sequence matcher tree.
	sequence_matcher.print_tree()
	pass # Replace with function body.

Answer 5 · 2023-03-04T06:14:29.000Z

This is a really good example indeed, thanks a lot for this.

However, it does raise a couple of issues.

I somehow overlooked FraySimpleInput.frombind(), assuming you had to use its builder. Given that you don't, why does it exist (or really, in what situation is the builder needed that I may be unaware of)?
Viewing FraySequencePath in this example makes it look like a singleton or a variable. Its quite unintuitive that newing the instance is a result of the from_ functions. I would suggest turning FraySequencePath.from().then() into something like FraySequencePath.create().from().then() to make it explicitly clear what is happening.
Documentation and naming surrounding FraySequenceList make it unclear what it is for. Should a sequence's name be the name of the move–and be specific to character, or simply an abstracted sequence–as implied by documentation within sequence_matcher.gd?

Answer 6 · 2023-03-08T21:24:57.000Z

I somehow overlooked FraySimpleInput.frombind(), assuming you had to use its builder. Given that you don't, why does it exist (or really, in what situation is the builder needed that I may be unaware of)?

from_bind is a shorthand for builder().bind(bind_name).build(). The full builder is still available if you want to set the priority or set it as virtual. But I found in testing that I was mostly just using simple inputs as a wrapper around a bind I wanted to use. So I thought this shorthand would be a useful inclusion.

Viewing FraySequencePath in this example makes it look like a singleton or a variable. Its quite unintuitive that newing the instance is a result of the from_ functions. I would suggest turning FraySequencePath.from().then() into something like FraySequencePath.create().from().then() to make it explicitly clear what is happening.

You think so? It's not super prevalent but I took inspiration from the Godot API for this convention. A few classes use the ClassName.from_something() convention. For instance Color.from_hsv() and GLTFCamera.from_node(). Or are you saying it is unclear in this specific case. I'll be resuming my work in a few days so I'm interested in your thoughts on this.

Documentation and naming surrounding FraySequenceList make it unclear what it is for.

Noted, I'll look into clarifying the sequence list documentation. I may actually rename things since it is very confusing after thinking about it. A SequenceList contains a list of sequences. A sequence is a name mapped to an array of SequencePaths where a SequencePath contains an array of InputRequirements... an array which basically the true "sequence" in all of this.

I'll give it some thought but maybe I could rename things as such:
SequenceList -> SequenceMap : Contains a mapping of string names to an array of sequences.
SequencePath -> Sequence : Contains an array of InputRequirements.

Should a sequence's name be the name of the move–and be specific to character, or simply an abstracted sequence–as implied by documentation within sequence_matcher.gd?

So, the name of the sequence is essentially just a label that you can assign to it, and it doesn't necessarily need to be based on character move names. Personally, I prefer to use "fighting game notation," which describes the sequence using button inputs, such as "236P" instead of "fireball." The way I see it move names and their associated sequences may be subject to change, but the actual sequences themselves will remain the same. Of course, this is all up to you and you can use whatever names you prefer. This is just a matter of design preference since none of this is enforced.

Answer 7 · 2023-03-09T02:09:44.000Z

Personally, I didn't find it too unintuitive, apart from the composite bind that I needed to search in the code to know how it work, but apart from that it was clear to me.

For composite bind, the only thing I truly needed was an example. from_bind() maybe not the best name, but I can't think of a better one.
I disagree on this point. create() is the same as from_first(). It's a workaround for variadic functions arguments, so for me it get a pass.
Maybe a better naming scheme would be SequenceTree and SequenceBranch ? You add a branch to the sequencetree is kinda a good memory trick ? There is also the function print_tree() that tell what kind of data structure you are using... There will never be a perfect name so a good example is a lot better.

Overall, I personally like how the code is structured.

Add input binding.
Create a bunch of SequencePath that you add to a SequenceList
Register the List to the Matcher, Bind user inputs to the matcher and register the match to a callback
React accordingly to the match.

Answer 8 · 2023-03-09T20:07:55.000Z

from_bind is a shorthand for builder().bind(bind_name).build(). The full builder is still available if you want to set the priority or set it as virtual. But I found in testing that I was mostly just using simple inputs as a wrapper around a bind I wanted to use. So I thought this shorthand would be a useful inclusion.

Gotcha. As long as in the documentation its introduced in its full form and the shorthand specified afterwards, I agree that seems useful. Maybe consider a slight name change? directly_from_bind()?

You think so? It's not super prevalent but I took inspiration from the Godot API for this convention. A few classes use the ClassName.from_something() convention. For instance Color.from_hsv() and GLTFCamera.from_node(). Or are you saying it is unclear in this specific case. I'll be resuming my work in a few days so I'm interested in your thoughts on this.

I mean this specific use case.
Something like Color.from_hsv() does indeed make sense because you pass it something (the titular hsv) as an argument and get something else back immediately. An instance is created, but conceptually it might as well be a cast, because you start with an instance, too. When you do not begin with an instance, calling the constructor or a well-named static function lets one understand an instance is being created.

However, while the "conceptually a cast" idea works for from_first(), it's never only one method call and something like FraySequencePath.from_first().then().then().then_all().enable_negative_edge() has zero indication of if/where there is an instance being created.

For instance, someone referring to the builders in the Input module may believe that a SequencePath is also requires "building" in the same way: FraySequencePath.from_first().then().build(), which of course is not the case. ~~I believe adding a method to indicate where instance initialization occurs would make interacting with it far more clear.~~
[EDIT] But it should be. After thinking on it, I believe making it consistent with the Input module is the best approach as doing so would be a very simple change.

Of course, this is all up to you and you can use whatever names you prefer. This is just a matter of design preference since none of this is enforced.

Of course, what was I was trying to ask was how it's intended to play along with the State Module.

which describes the sequence using button inputs, such as "236P"

so to get it straight, you describe the 236P input sequence with a StringName key "236P", and multiple character's ...StateMachines can reference that same "236P" input sequence via the key for their respective, unique moves, because they have the same input?

Overall, I personally like how the code is structured.

Don't misunderstand, I do too! I think the method chaining is really flexible approach that doesn't compromise readability.
However, there's just a couple of inconsistencies and small improvements left to be made.

Answer 9 · 2023-03-18T17:53:29.000Z

First off just wanted to thank both of you for giving the framework a try and contributing to the discussion. I've addressed some of your comments in my latest PRs and I'm in the process of updating the documentation which should be completed later today or tomorrow.

I disagree on this point. create() is the same as from_first(). It's a workaround for variadic functions arguments, so for me it get a pass.

@Remi123 I was also a fan of the from_first() method but @20milliliter has convinced me it should be replaced by a builder for consistency with the rest of the framework.

Changes can be viewed in this PR: #30

Maybe a better naming scheme would be SequenceTree and SequenceBranch ? You add a branch to the sequencetree is kinda a good memory trick ? There is also the function print_tree() that tell what kind of data structure you are using... There will never be a perfect name so a good example is a lot better.

@Remi123 Thank you for the name suggestions, I've decided to use them since they're reflective of the underlying data structure. And I agree, clearer examples will be more useful than any name changes.

Changes can be viewed in this PR: #30

I mean this specific use case.
Something like Color.from_hsv() does indeed make sense because you pass it something (the titular hsv) as an argument and get something else back immediately. An instance is created, but conceptually it might as well be a cast, because you start with an instance, too. When you do not begin with an instance, calling the constructor or a well-named static function lets one understand an instance is being created.

@20milliliter You've convinced me, I've replaced the from_first from a static builder method. It does function as a builder rather than a converter so the name is a bit misleading.

Changes can be viewed in this PR: #30

Of course, what was I was trying to ask was how it's intended to play along with the State Module.

@20milliliter Ah I see, I misunderstood. I may still be misunderstanding so if it seems i'm not answering your question or giving you redundant info please let me know. Inputs in the state module are independent from the input module. The two modules communicate via string names but one does not reference the other. The state machine doesn't care how you know "236P" was inputted, just that a string with that name was given. I assume you're already aware of this but I just wanted to make things clear just in case.

With that being said here is how I'd suggest the two playing along.

Each fighter should have their own state machine, an sequence matcher. The matcher can be initialized with all the sequences you intend for the fighter to use. If fighters largely share sequences you could even create one "mega sequence list" that is used to initialize every matcher.
You then setup state transitions for the fighter based on the sequence names you used in the list. I recommend coming up with some naming pattern that reflects the inputs used to trigger the sequence (as I've said before I use fighting game notation). You could use move names but I prefer thinking in terms of "when sequence perform action" rather than "when action perform action" if that makes sense. I also prefer to avoid "coupling" an action with a specific sequence via the name since the sequence will generally stay the same but the action is subject to change.

Aside from updating outdated example code, perhaps it needs a little work to add clarification on certain things (ex. why are some class factories separate while others are internal?), or slightly better naming (ex. FrayStateNodeStateMachine and related).

@20milliliter Regarding an older comment of yours I've gave it some thought and made some changes to the state module to improve the naming and make things consistent with the rest of the framework.

Changes can be viewed in this PR: #29

Answer 10 · 2023-03-18T21:14:52.000Z

With that being said here is how I'd suggest the two playing along.

Gotcha, that makes sense.

Thank you so much for your work on this!
It will be a bit before I am able to get my hands dirty with this again but I'm looking forward to it.

Answer 11 · 2023-03-19T07:24:45.000Z

Closing since documentation updates, as well as the changes I mentioned in my last comment, have all been merged! Again thank you for the feedback from you two. Everything can be see in the latest commit 5a02428

I undoubtedly missed some things or caused a regression but I think I'll be packing this up as an alpha release soon. Once I'm satisfied with the API changes and resolve any issues I spot I'll see where things go from there.