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.

Sign up for GitHub

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

Jump to bottom

Replace // with \\ before sending path to Blender #85335

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Replace // with \\ before sending path to Blender #85335

merged 1 commit into from
Jan 9, 2024

Conversation

zinefer
Copy link
Contributor

@zinefer zinefer commented Nov 25, 2023

Why?

I was having an issue using the blend importer and realized it was the fact that I keep my files on a network drive. Using wireshark I was able to see both the http RPC call and the SMB traffic and could tell that Blender was attempting to open a file at //fileshare/game/fileshare/game/assets/model.blend and not finding it. I can see that the path sent is //fileshare/game/assets/model.blend but unfortunately, without the backslashes, Blender just treats it as a relative path.

Interesting too, because c_escape() is being called which says to me that someone expected this to return backslashes.

Then I noticed in ProjectSettings that \ is replaced with /.

What?

Instead, when the path begins with //, replace the first two chars with \\ which when escaped becomes \\\\.

Resolves #85334

@zinefer zinefer requested a review from a team as a code owner November 25, 2023 04:23
@akien-mga akien-mga added bug platform:windows topic:import cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 25, 2023
@akien-mga akien-mga added this to the 4.3 milestone Nov 25, 2023
@YuriSizov YuriSizov changed the title Bugfix: Replace // with \\ before sending path to Blender Replace // with \\ before sending path to Blender Nov 27, 2023
@zinefer
Copy link
Contributor Author

zinefer commented Dec 5, 2023 •
edited
Loading

I just wanted to double check, is there anything else I can do to improve this? I considered an integration test but since it's an interaction between Godot, Blender and a Windows file share, it might be more trouble to maintain than it's worth.

@fire
Copy link
Member

fire commented Dec 5, 2023

@RedMser Looking good?

@AThousandShips AThousandShips changed the title Replace // with \\ before sending path to Blender Replace // with \\ before sending path to Blender Dec 5, 2023
@RedMser
Copy link
Contributor

RedMser commented Dec 5, 2023

@zinefer Judging by the discussion above, you have not compiled the code to see if your change actually fixes the problem?

If you don't want to build Godot locally (although it's recommended to set that up on your PC for easier testing), you can download a build from GitHub by going to this PR's "Checks" tab at the top, then in the top right there is a dropdown "Artifacts".

@zinefer
Copy link
Contributor Author

zinefer commented Dec 5, 2023 •
edited
Loading

you have not compiled the code to see if your change actually fixes the problem?

I have tested the Blender side by calling the RPC with a modified payload but I have not done E2E testing. 🙊

you can download a build from GitHub by going to this PR's "Checks" tab at the top, then in the top right there is a dropdown "Artifacts".

Great tip! I will report back. Thanks for all the help everyone. 🤩

@zinefer
Copy link
Contributor Author

zinefer commented Dec 6, 2023 •
edited
Loading

I have tested it E2E and with the latest commit, things are working! 🎉

Here's a screen recording:

2023-12-05-17-43-28.mp4

akien-mga
akien-mga reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good! Could you squash the commits? See PR workflow for instructions.

modules/gltf/editor/editor_scene_importer_blend.cpp Outdated Show resolved Hide resolved
modules/gltf/editor/editor_scene_importer_blend.cpp Outdated Show resolved Hide resolved
@zinefer zinefer force-pushed the bugfix-blend-importer-on-windows-network-share branch from dc17496 to b8e3460 Compare December 7, 2023 22:11
@AThousandShips
Copy link
Member

You didn't squash your commits, you just picked the first one, you need to restore what suggestions were made

@zinefer
Copy link
Contributor Author

zinefer commented Dec 7, 2023

Yeah, I see that, I'm working on it! Thanks for the fast assistance!

@AThousandShips
Copy link
Member

AThousandShips commented Dec 7, 2023 •
edited
Loading

You can do: git reset --hard dc17496 and try again

Though this likely happened because you forgot to pull the changes to your computer

@zinefer zinefer force-pushed the bugfix-blend-importer-on-windows-network-share branch from b8e3460 to ab0755a Compare December 7, 2023 22:18
@zinefer
Copy link
Contributor Author

zinefer commented Dec 7, 2023

Failing style checks now... On it. 😰

@zinefer
Bugfix: Replace // with \\ before sending path to Blender
72d18d5 
On Windows, Blender treats //fileshare/assets/model.blend as a relative
path which will not be found. Instead, replace the first two chars with
`\\` which when escaped becomes `\\\\`.
@zinefer zinefer force-pushed the bugfix-blend-importer-on-windows-network-share branch from ab0755a to 72d18d5 Compare December 7, 2023 22:23
@zinefer
Copy link
Contributor Author

zinefer commented Dec 7, 2023 •
edited
Loading

Oof! I sure made a mountain out of that molehill. 😅

I think we're good to go now. A single commit with:

  • A period on the end of my long comment
  • Properly indented ifdefs
  • substr(2) over right(-2)
  • Certified backslashes
  • Passes style checks
  • Compiles

Once the pipeline finishes, I'll download the executable and test it against my project just to be extra careful since I have already mucked this up lol.

@zinefer
Copy link
Contributor Author

zinefer commented Dec 9, 2023 •
edited
Loading

I just downloaded and successfully tested this build. 🚀

@akien-mga akien-mga merged commit 78fadf4 into godotengine:master Jan 9, 2024
15 checks passed
@akien-mga
Copy link
Member

akien-mga commented Jan 9, 2024 •
edited
Loading

Thanks! And congrats for your first merged Godot contribution 🎉

@zinefer zinefer deleted the bugfix-blend-importer-on-windows-network-share branch January 14, 2024 14:53
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

@aaronfranke aaronfranke aaronfranke left review comments

@RedMser RedMser RedMser approved these changes

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
bug platform:windows topic:import topic:3d
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Blend importer fails when assets are on Network Share
8 participants
@zinefer @fire @RedMser @AThousandShips @akien-mga @YuriSizov @aaronfranke @Calinou