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

Verify that scene load issue is gone #20

Closed
Abb4 opened this issue Aug 7, 2023 · 5 comments · Fixed by #23 or #25
Closed

Verify that scene load issue is gone #20

Abb4 opened this issue Aug 7, 2023 · 5 comments · Fixed by #23 or #25
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Abb4
Copy link
Contributor

Abb4 commented Aug 7, 2023

In #5 an issue with loading checked-out godot scenes was reported. Apparently NavigateTowards2dNodes.cs script was not included correctly. We suspect malformed godot scene files. Re-including the script into the nav_mesh_2d_seeker.tscn godot scene seemed to fix the issue.

On my pc I was able to start the project without issues.

If the issue persists after future pull requests it should be investigated further

@Abb4
Copy link
Contributor Author

Abb4 commented Aug 7, 2023

@softwaredev101 I just merged a tiny pr: #21

Could you verify that the scene loading issue is gone with current main? Before we get the issue with massive prs?

I have looked into the scene files and I just can not figure out why you get the issue. If I remove the script, save the scene and reassign it, and save the scene, I get no changes in git. This means that the asset files (here specifically NavigateTowards2dNodes), whenever they are assigned/imported, always get the same id.

I suspect two causes for this behavior:

  1. (script-) files cache their ids somewhere in the project, outside of scene files
  2. Ids are not fully randomly generated

As for 1.: I could not find the file where these Ids are cached before being used in scene files. Therefore I assume 2.
As for 2.: I could imagine that godot hashes resource files and generates the Id from that. This means that including the same resource two times will always result in the same id.

I believe this was not always the case as evidenced by godotengine/godot-proposals#471
In godot 4 id generation was changed to favor git friendly formats: godotengine/godot#50676

I tried to understand the changed code but my brain is too smooth for cpp.

I propose we investigate further if the issue arises again. If it does not we can close this with clear conscience

@softwaredev101
Copy link
Collaborator

softwaredev101 commented Aug 7, 2023

@Abb4 Thanks for looking into this problem
Unfortunately, the crashes persist. Regarding your statements about Git, it is normal. If you detach a script from the corresponding node and reattach it, no changes have been made since you've been going back to the beginning. Regarding the C++ code I can extract the following insights:

  • You first create a hash from the number of ticks, all of the components of the currently fetched date (here: year, month, day, hour, minute and seconds) and use the DJB2 hash for it. For more information, one can look at this resource
  • You're right that the second part (inside the for loop) is quite cryptic so far. However we should keep in mind that its "pseudo" unique. Therefore I would create a unique name for each component. For instance, if I want to create a script which manages the Guard AI I call this script GuardAI.cs instead of the Node name Guard since I would risk running into this problem once again.

From my perspective changing the Script name to another name than the NavigateTowards2dNodes solved the problem after crashing once again (yeah, I know it's likely not ideal). Keep in mind that renaming scripts may break the corresponding nodes since we need a 1:1 mapping from class name to script file name

@Abb4
Copy link
Contributor Author

Abb4 commented Aug 7, 2023

God damn it, It does cache 🙈

I forgot about .gitignore.

In .godot folder, which is excluded from git there seem to be changes after fixing the packed scene. Now I need to find the changes, which cause the crashing and include them in the commit log to avoid packed scene issues.

@Abb4
Copy link
Contributor Author

Abb4 commented Aug 7, 2023

One of the changes in 592ebab should be the culprit.

@Abb4 Abb4 added this to the Release v1.0 milestone Aug 8, 2023
softwaredev101 added a commit that referenced this issue Aug 10, 2023
Abb4 added a commit that referenced this issue Aug 10, 2023
@Abb4
Copy link
Contributor Author

Abb4 commented Aug 10, 2023

I still need to find the file inside .godot that we need to keep.

@softwaredev101 If we merge as it currently is, you get a bunch of my internal editor settings every time we sync work (e.g. currently selected scene)

@Abb4 Abb4 reopened this Aug 10, 2023
Abb4 added a commit that referenced this issue Aug 10, 2023
…s downwards by searching appropriate component (fixes #20)
Abb4 added a commit that referenced this issue Aug 10, 2023
…s downwards by searching appropriate component (fixes #20)
Abb4 added a commit that referenced this issue Aug 10, 2023
…s downwards by searching appropriate component (fixes #20)
Abb4 added a commit that referenced this issue Aug 10, 2023
…s downwards by searching appropriate component (fixes #20)
@Abb4 Abb4 closed this as completed in #25 Aug 13, 2023
Abb4 added a commit that referenced this issue Aug 13, 2023
…rence

removed `NavigateTowards2dNodes` attribute, instead push parent values downwards by searching appropriate component (fixes #20)
@Abb4 Abb4 added the bug Something isn't working label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment