- Keyboard Shortcuts#30
Conversation
There was a problem hiding this comment.
Thanks for submitting! Great work so far! I've done a first pass and left some comments :)
One general comment is that upon an execution of an operation, we need to update the underlying AST and clear up any open editors in the dock. As you mentioned on the linked issue, this means we need to notify the main_panel that something has happened. The implementation for this probably depends on the outcome of some of my comments - happy to help when these are sorted/agreed :)
Some thoughts for the future:
- Testing this will be interesting - if it's not easily possible, no problem at all but it will be interesting to see what we can do with GUT!
- We'll need to think about adding some documentation as well. Nothing major, just something to say what keyboard commands are available etc. Again, happy to show you how the docs work when the time comes :)
Any questions, give me a shout!
|
Apologies for the delay - very busy week! I have time this weekend and tomorrow afternoon to have a further play with it :) |
No worries, i couldn't find time to continue my work on this anyways. Maybe this weekend i will get some stuff done |
|
so here is a question, I managed to delete a node successfully and save the dialogue ast. But undoing it will require for me to re-add the node to the dialogue ast and I couldn't find a capable method for doing that. only method i found is |
Thanks and absolutely no rush on your end as well :) |
Good question! If storing as an AST, I would recommend defining a new method that accepts func add_node_from_ast(node_ast: ParleyNodeAst, insertion_index: int) -> ParleyNodeAst:
# Probably need to make sure the provided ID doesn't already exist
# Also worth storing the insertion index somewhere as well?
nodes.insert(insertion_index, node_ast)
_emit_dialogue_updated()
return node_astwdyt? |
i was going to store the nodes itself without queuefree them. but storing asts makes much more sense. Only problem is that i don't have access to the ast from the parleygraphnode itself. if there was a variable inside maybe i could do that. or let me know if there is a way to get ast from graphnode. |
|
Very good point - we don't currently store the AST data within the graph nodes themselves. We do have the ID and we have access to the Dialogue Sequence AST in One other thing I've just thought of that we may need to consider is: providing dialogue sequence context to the history. For example, what if we?
wdyt? |
|
Okay i will use the id to get the ast. But to make things loosely coupled I prefer handling the operations inside the operation. If we need some components or scripts we can insert them when creating the operation. |
Agreed - nice one!
I believe it would as it should call the |
Does insertion_index matter? Couldn't we just append the node_ast? As i understand, the nodes array in dialogue_sequence_ast.gd are handled with like push_back functions which doesn't care the index. |
Good question! Atm the nodes array is in order of node creation, so I was thinking that if you delete a node in the middle of an array and then undo this deletion, it's gonna cause some unexpected git diffs in the output dialogue sequence JSON (.ds) file because the re-added node would be in a different position in the file. It's not the end of the world, and certainly a nice to have, because as you point out, it doesn't affect functionality - I don't have a strong opinion either way on this! :) In the future, we could probably add some kind of ordering to the nodes array in the JSON output but that's probs out of scope for this PR! On a side note, I also want to spend some time looking into using dictionaries under the hood as this will be keep performance good at scale |
|
okay then, for now ill just append the nodes for now. It might be better to sort them later because if a bulk deletion is requested it will be very hard to manage the indexes of multiple nodes at the same time |
Sounds good to me! |
- undo redo history grouped by dialoge sequence ast
|
I have finally finished the delete operation and undo redo functionalities are working smoothly. even between two different dialogue sequence. Needed to add some get or find methods to get access to ast's. Let me know your feedbacks |
Awesome stuff! I will have a look asap |
There was a problem hiding this comment.
Hi! Really good progress! :) Just reviewed the code now and left some comments - let me know what you think!
I also had a play with your changes just now as well and I couldn't get the redo/undo to work (delete is fine) - I got the following errors in the console while playing around with: all.ds - have you come across these errors?
I also noticed a slight bit of colour weirdness when creating a new edge, every out going connection turns blue instead of staying light green. I don't know if you've noticed this?
Just FYI, I'm starting a new job next week so I may be slow to respond over the next week but I will have some time on weekends :)
| elif key_event.keycode == KEY_DELETE: | ||
| delete_selected.emit() | ||
|
|
||
| func _handle_mouse_select(mouse_event: InputEventMouseButton) -> void: |
There was a problem hiding this comment.
Just looking at this function logic in detail, it seems a little quadratic. I wonder how this will behave for large dialogue sequences - especially since it's running on every left click event, which is a very common operation.
As a result, I suspect we may have performance issues with this, so to be on the safe side, I'm thinking that maybe connection selection is a feature we could maybe turn off in project settings and allow users to opt-in?
wdyt?
There was a problem hiding this comment.
maybe if we cached the connections it would be better. i haven't experienced a performance issue, all though i don't have a low-end computer to test it.
Selecting and searching through connections can be simplified with an id but i would have to generate id for each connection. ideally it would have been nice to integrate this with the parley_edge_ast since that has also generated id but that is a task that would require a lot of time to finish at least for me.
There was a problem hiding this comment.
cool, if you haven't noticed a performance issue so far then that's good news - I've got a few large Dialogue Sequences so I'll see how they fair - I'll report back :)
ideally it would have been nice to integrate this with the parley_edge_ast
Agreed, something for the future for sure!
| var previousToColor: Color | ||
| var selected: bool | ||
|
|
||
| func _init(_edge_ast: ParleyEdgeAst, _from_node: ParleyGraphNode, _from_port: int, _to_node: ParleyGraphNode, _to_port: int) -> void: |
There was a problem hiding this comment.
Nit: in _init methods, I think it's generally good practice to add a default value to the parameters so you can in theory call it as follows: ParleyGraphEdge.new() - I can't recall where I read this, but I think I read it in the gdscript tutorial
There was a problem hiding this comment.
i do think it could be misleading. if someone forgets to put parameters it will still compile but will give errors on runtime since the parameters are expected to have a value
There was a problem hiding this comment.
i do think it could be misleading.
See the docs comment here, this is the thing I remember reading: https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-private-method-init
So if we were to call duplicate on this class, it would fail. I think in this case, I generally prefer to use factory methods for this. I do take your point on it being misleading however.
So, if we're only ever instantiating this directly then all good! But maybe worth adding a comment to illustrate this just in case :)
There was a problem hiding this comment.
Alright I'll add a comment
I see! Can we fix this behaviour so it doesn't change the colour of related edges when creating new edges using click and drag? Cheers and as always, lemme know if I can help!
Cool cool! Lemme know if you need me to test anything specific :)
I think based on your code it should be supported provided I used ctrl+z but it seemed to be erroring for me - but I could be wrong! Give me a shout when Cmd+z (etc) support is added for Mac and I can re-test :) In the meantime, I'll dig into why this is happening as well since I can replicate the error |
|
Just FYI, I'm almost done with translation support here - I don't think there are any conflicts with your PR but I thought I would mention incase you wanted to have a look :) #28 |
|
sorry for the delay i was super busy these couple of weeks. as for the progress, i implemented the macos support and learned that port color doesn't exist. it's a default behavior by godot to change all the connections one side color when you set the slot color. there might be an advanced solution which i am not aware of. Would like to see if you encounter any more errors after this last commit. |
Co-authored-by: Jonny Green <me+github@jonnydgreen.com>
|
I have done your suggested changes. also i have committed and improved edge selection with bezier calculation. And i didn't imagine it would work this smoothly, you will see the result yourself :D |
This comment was marked as outdated.
This comment was marked as outdated.
|
cool! Taking a look now! :) |
There was a problem hiding this comment.
Great stuff and outstanding work with the bezier curve! As you say, we are getting there! 🚀 (And of course, no rush on the copy/paste work!)
I haven't finished my review yet as I've mainly focussed on the duplication operator this evening - I'll continue this over the weekend as I'm busy every other evening this week unfortunately!
In the meantime, I had a play and it's working nicely at first glance:) I did notice the following issues, I don't know if you've come across them?
- Duplication nodes don't copy values
- Weirdness happens with the edges when I do the following on
all.ds:
a. Duplicate node 27
b. Add edge from 27 -> 29
c. Undo change
d. ==> Weirdness with edges - Duplicating with multiple selections didn't work for me
|
done your suggested changes and added copy paste support! please take a look |
Fantastic! :) I will take a look this week and answer all your other Q's! |
There was a problem hiding this comment.
Hello! Happy March! :) This is soooo good! I've been having a load of fun with it today! We are very close now!
Thanks for implementing the copy/paste functionality, I've just tried it out now! 🚀 On the whole it's working great! I've had a few intermittent issues with it unfortunately and am unable to create edges on copied nodes. Here are my steps to reproduce, lemme know what you think:
- Copy node or duplicate node
- Try and connect an edge to it
I believe the issue is with the use of duplicate and references. Having looked in depth at this, I think we will need to adjust to a more reliable approach as follows (this also gets rid of some of the other issues I was seeing):
I'm also seeing a few other intermittent issues crop up here and there which are hard to reproduce - (mainly when testing the undo/redo functionality and playing around with edges). Given this and the best kind of testing is using this for real, I think we should put this behind a feature flag in the Parley settings as an opt-in feature (I'm gonna do this with my translations support PR as well). With this in place, this will allow us to release this very soon and once we are fully confident, we can switch to on by default! :)
As I mentioned in my previous comment on this, this can be done in: parley_settings.gd and constants.gd (if you agree, I recommend: "parley/editor/keyboard_shortcuts") but lemme know if you need a hand or guidance with this
As I understand it, we have the following left to do:
- Fix any remaining bugs/implementation details
- Tidy up code, and remove/fix any prints/todos
- Fix any GDScript warnings/errors
- Put this behind a feature flag and set to opt-in (add a TODO to switch this to opt-out)
- Testing - after which, I can do a final bit of review polish
- Documentation (can be done in a follow-up PR - I'm happy to do this if you'd prefer)
Anything I've missed for what is left to do? :)
I'll aim to spend Thursday/Friday evening of this week doing some further testing this feature and get us ready for merging - really, really well done!! And thank you so much! 🚀
| id = new_id | ||
| elif not id or id == id_prefix or not id.begins_with(id_prefix): | ||
| id = new_id if new_id.begins_with(id_prefix) else "%s%s" % [id_prefix, new_id] | ||
| #endregion |
There was a problem hiding this comment.
I think this may have been accidental, could we revert this please - thanks :)
There was a problem hiding this comment.
copy function is not needed anymore and removed but setting the id externally still requires this change, i believe on of your comments you made a suggestion of changing comment on the id setter so i take it this is resolved
There was a problem hiding this comment.
ah sorry - I was referring to the deletion of the #endregion only - the other changes are all good :)
| if arrange: | ||
| arrange_nodes() | ||
|
|
||
| call_deferred("refresh_edges") |
There was a problem hiding this comment.
Is this addition needed? It's unfortunately conflicting with some of the existing refreshing functionality (e.g. unexpected nodes get reselected after clicking the refresh button) so I'd be tempted to remove it if at all possible! Or we can put it behind the feature flag :)
There was a problem hiding this comment.
it was to restore the states of selected edges after undoing. if you could tell me your scenario i can make a fix i think
There was a problem hiding this comment.
I can't remember the scenario unfortunately! To be on the safe side, could we add this behind a feature flag? Thanks! :)
| call_deferred("refresh_edges") | |
| if ParleySettings.get_setting(Constants.EDITOR_IS_KEYBOARD_SHORTCUTS_ACTIVE, false): | |
| call_deferred("refresh_edges") |
Co-authored-by: Jonny Green <me+github@jonnydgreen.com>
Co-authored-by: Jonny Green <me+github@jonnydgreen.com>
- copy shortcut fixes and comment changes
|
I send my changes. Added the keyboard shortcuts opt-in in project settings as you mentioned. |
Fantastic, thank you so much! I will take a look this week :) |
|
Hello! Apologies for the delay in reviewing this - I had/have to focus on some other things outside of OSS but I should be all good to test, review, and release this in a couple of weeks! :) |
|
Back now! Apologies, that took longer than expected - I've blocked out some time this Sunday onwards to get back into the swing of things and we can get this released! 🚀 |
|
Code looks fab, awesome work! Gonna test it out over the next couple of days and go over in a bit more detail but I think we're pretty much there! 🚀 |
There was a problem hiding this comment.
Fantastic! Looking over the bezier stuff again was great! 🔥 I've left a few comments to tidy up the code a bit and provide a bit of extra safety, but I think we are there!
The only thing remaining is some documentation but that can be a follow-up PR at some later time - wext stop is merge (squash) and release I think! Well done and thank you!
| if arrange: | ||
| arrange_nodes() | ||
|
|
||
| call_deferred("refresh_edges") |
There was a problem hiding this comment.
I can't remember the scenario unfortunately! To be on the safe side, could we add this behind a feature flag? Thanks! :)
| call_deferred("refresh_edges") | |
| if ParleySettings.get_setting(Constants.EDITOR_IS_KEYBOARD_SHORTCUTS_ACTIVE, false): | |
| call_deferred("refresh_edges") |
#endregion added back. Co-authored-by: Jonny Green <me+github@jonnydgreen.com>
Co-authored-by: Jonny Green <me+github@jonnydgreen.com>
…/parley into keyboard-shortcuts
Co-authored-by: Jonny Green <me+github@jonnydgreen.com>
|
Suggested changes are done! |
|
Fantastic! I'll merge and release shortly! Thank you again!! 🚀 |
|
Submitted the edit to the godot asset library - should be live over the next days or so :) |
|
Nice work! |



No description provided.