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

1.x Speedups #1271

Merged

Conversation

exelia-antonov
Copy link
Collaborator

Various fixes I've had in mind for improving Dialogic 1.x speed, which I had initally put aside when the Dialogic 2 development started but they're good for long term maintenance for people that for one reason or another can't move off of Godot 3

@exelia-antonov
Copy link
Collaborator Author

First improvement:

  • Dynamic cache for the built-in events. will load them only as used, then keep them in memory and pull them from the cache as needed. doesn't cache the custom events, just because they could changed much more commonly then the Dialogic core events

@exelia-antonov
Copy link
Collaborator Author

starting to address #1239, doing some initial observations as I try and step through from that. I've added timestamps to build_resource_folder() in the MasterTree.gd, since i gotta work backwards through all the steps to figure out whats happening where before I can do the full improved internal timeline/variable address dictionary changes

Umm

image

creating one single timeline folder probably shouldn't be calling build_resource_folder() a total of 22 times...

`

@exelia-antonov
Copy link
Collaborator Author

wait... omg... it's calling that for every folder... the count of calls goes up for each new folder created. no wonder it keeps getting slower and slower!

@exelia-antonov
Copy link
Collaborator Author

Further observations tonight: every update_folder_structure() first loads all the firles, loads the folder strucutre JSON, does the recursive building, and then saves the JSON

image

with the comments syaing 'when files got deleted and on program start', its called once in _ready() on the main editor, once in _on_RemoveConfirmation_confirmed()... then once in each of the build functions: build_timelines(), build_characters(), build_definitions(), build_themes()

the read in first before saving seems a bit redundant to me, so might readjust that. really should only genuinely happen in the editor on startup, and all the other functions be write-only.

@exelia-antonov
Copy link
Collaborator Author

note to myself since I need to leave: the folder properties metadata isn't included in how this is built at the moment. needs to be added for Editor to use both for folder colors, as well as for preserving empty folders. can then be stripped out later for the runtime dictionaries because they don't matter there. also will need to add some additional sorting properties for editor, for when its exploded back into the folders.json organization

might be possible to add finer drag and drop sorting into the folder list with what I have in mind, though, so you can actually rearrange timelines in the same folder

@exelia-antonov
Copy link
Collaborator Author

ok FINALLY, lol. was really busy and couldnt focus a lot of time, but also recursion suuuuuuucks lol

image (4)

now that the framework for both reading in to new file structure, then writing back again to folder_structure.json, I can clean it up and start hooking all the rest of the parts of 1.x into it

@Tummyache
Copy link

I just made a maze generator with path finding, so I feel that recursion hate. Can't wait for the fix, really appreciate the effort!

@exelia-antonov
Copy link
Collaborator Author

ok hmm trying to put the rest of what's in DialogicClass.prepare() (which is where I was doing it only temporarily for step testing) into DialogicResources.get_resource_folder_flat_structure() makes the whole thing crash with cyclic errors, which seems to be a static typing issue with GDScript in 3.x, gotta think through that a bit differently then

@exelia-antonov
Copy link
Collaborator Author

reminder to myself to check what happens here when you actually have no name in the timeline file

image

@exelia-antonov
Copy link
Collaborator Author

todo:
make new files go down to the first item in the folder, so it doesn't appear above subfolders (it rebuilds on project reload to be in that position)

@exelia-antonov
Copy link
Collaborator Author

reopened the bug #1278 just so i can do a verify check to make sure that works propelry with these changes

@exelia-antonov
Copy link
Collaborator Author

need to be able to reverify colors in Characters if somehting changes, becuse otherwise it'll revert until full reload

@exelia-antonov
Copy link
Collaborator Author

reminder to myself that I will need to add some checks and documentation because this introduces the minorest of breaking changes that forward slashes will be disallowed in folder and file names and need to be stripped on names and renames - if they aren't already? might be alrady idk I'll need to check

@exelia-antonov
Copy link
Collaborator Author

todo: make sure loading saves works

@exelia-antonov
Copy link
Collaborator Author

exelia-antonov commented Mar 10, 2023

current things needing to be addressed before this can be merged:

  • new definitions/glossaries are not being added to subfolders, but to root
  • correcting where new files of each category go if the new timeline/character/definition/glossary/theme button is pressed when that's not the current highlighted tree
  • the warning about forward slashes in names of folders and files, and the stripping of them for the breaking change
  • strip the folders from the trees at runtime, to prevent errors

current things that would be nice too, but I'm not marking them as blocking:

  • the dropdown menus in editor for timeline events to use the new tree instead of older recursive tree
  • trying to restore jumping to the right position in the tree on new item creation, and automatically going into rename mode
  • restoring the actual event parts cache, since that was the beginning of this patch but something weird with instancing in Godot 3 makes it preserve values on reinstancing (I'm using the exact same dang method in Godot 4 for the Dialogic 2 event cache, and it doesnt do the same thing there??)

@exelia-antonov
Copy link
Collaborator Author

TODO: since I found a much simpler way of doing the dropdown menus, gotta do it for the other pickers too

@goblinJoel
Copy link

goblinJoel commented Jul 5, 2023

Not sure how actively this is being worked on right now (thanks for all the work so far!), or if I'll be upgrading to 2.x anyway before I finish development, but:

I noticed that 1.4.5 is loading every character file, apparently from disk, every time any character is looked up. (DialogicUtil.get_character_list(), which is called by get_character(id), get_sorted_character_list(), and get_characters_dict().) Does this PR already address that?

@zaknafean zaknafean mentioned this pull request Jul 16, 2023
@exelia-antonov exelia-antonov marked this pull request as ready for review July 18, 2023 14:05
@exelia-antonov exelia-antonov merged commit ac656b6 into dialogic-godot:dialogic-1 Jul 20, 2023
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.

None yet

3 participants