Fix .blend files with quotation marks in filename fail to import

[Hilderin]

• Fixes .blend files with quotation marks in filename fail to import #78447

The main problem was using c_escape for paths which add \ to ' resulting to an invalid path since \ is not an escape character in XML.

Also, I added a parse and print for faultString if an error occur in Blender/Python.
I also fixed the problem where every call returned 'cannot marshal None unless allow_none is enabled' because the Python method export_gltf did not return a value.

Tested:

• Filename with '.
• Filename with accents (non standard characters).
• Export without RPC for filename containing ' (do_import_direct).

Example of the blender error message from RPC response:

[akien-mga]

Note that the String class has xml_escape and xml_unescape if we need to ensure proper escaping.

[Hilderin]

The values are already escaped in dict_to_xmlrpc and this function is used to created the XML payload.

It is important to not escape strings in parameters_map and request_options because they will be double escaped. And more, the same dictionary is used for rpc call (xml) in do_import_rpc and for direct call in do_import_direct where the values are converted in python and escaped in dict_to_python. Another c_escape is made in dict_to_python that was causing a double escape for the ' when not in RPC.

Just to be sure, I tested with a &:

[RedMser]

Changes make sense to me, and hopefully that covers all edge cases.

Thanks for the error handling code as a bonus, that's very nice :)

[Hilderin]

I just add a little problem with this PR on my computer. I had 2 versions on Godot running, one with this modification, and one without. If the version without did a blender import first, blender was started with the old script that returns 'cannot marshal None unless allow_none is enabled'. When I import a file in Godot with this PR included, the error is trapped and the importation fails. I had to close the old version before it works again. Both version use the same blender process with the old script.

I think I should discard the error 'cannot marshal None unless allow_none is enabled' just to prevent issues for users using different versions of Godot? What do you think?

[RedMser]

I think I should discard the error 'cannot marshal None unless allow_none is enabled' just to prevent issues for users using different version of Godot?

Not sure if it's worth adding this check, for the rare case of having multiple Godot instances running and using Blender imports on both...

I suppose if the check is simple enough to implement, documented with a comment as to why it's needed, it won't do any harm to add it. At least the error message is hard-coded on Python's side ^^

[Hilderin]

There you go, I just added a check to discard the error 'cannot marshal None unless allow_none is enabled'.

[demolke]

I was investigating #93997 and was digging through this code yesterday.

I wrote a similar thing but didn't finish it and saw yours today - demolke@7533dcd

[demolke]

This only works if the stream can be read in one go.

[Hilderin]

Following your comments, I used an easier method to determine if the exportation is successful or not. I just look if the string 'BLENDER_GODOT_EXPORT_SUCCESSFUL' is present in the output. That way, I don't have to parse the XML and the success message should always be in the first chunk. Worst case scenario, the error message is incomplete which, I think it's not a big deal (we did not have any error message before).

[demolke]

If you're introducing a string to look for, I think it would be safer to read the whole buffer. It's not too much extra work.

Something like

	// Wait for response.
	bool done = false;
	PackedByteArray response;
	while (!done) {
		status = client->get_status();
		switch (status) {
			case HTTPClient::STATUS_REQUESTING: {
				client->poll();
				break;
			}
			case HTTPClient::STATUS_BODY: {
				client->poll();
				response.append_array(client->read_response_body_chunk());
				break;
			}
			case HTTPClient::STATUS_CONNECTED: {
				done = true;
				break;
			}
			default: {
				ERR_FAIL_V_MSG(ERR_CONNECTION_ERROR, vformat("Unexpected status during RPC response: %d", status));
			}
		}
	}

	String response_text = "No response from Blender.";
	if (response.size() > 0) {
		response_text = String::utf8((const char *)response_chunk.ptr(), response_chunk.size());
	}

	if (client->get_response_code() != HTTPClient::RESPONSE_OK) {
		ERR_FAIL_V_MSG(ERR_QUERY_FAILED, vformat("Error received from Blender - status code: %s, error: %s", client->get_response_code(), response_text));
	} else if (response_text.find("BLENDER_GODOT_EXPORT_SUCCESSFUL") < 0) {
		// Previous versions of Godot used a Python script where the RPC function did not return
		// a value, causing the error 'cannot marshal None unless allow_none is enabled'.
		// If an older version of Godot is running and has started Blender with this script,
		// we will receive the error, but there's a good chance that the import was successful.
		// We are discarding this error to maintain backward compatibility and prevent situations
		// where the user needs to close the older version of Godot or kill Blender.
		if (response_text.find("cannot marshal None unless allow_none is enabled") < 0) {
			ERR_FAIL_V_MSG(ERR_QUERY_FAILED, vformat("Blender exportation failed: %s", response_text));
		}
	}

	return OK;
}

[demolke]

Actually that's wrong, let me check what's the right way :)

[demolke]

Updated the comment above

[Hilderin]

Thanks a lot for this code. I never worked with Godot HTTPClient before. It seems to work perfectly.

[demolke]

Why not return Error and then you can propagate the result out of do_import_rpc()

[Hilderin]

Good point! But this method does not exists anymore.

[demolke]

When I was looking at it, I thought I would just grab all the text elements and avoid the nesting/replicating the xml structure in code.

[Hilderin]

Good idea, that made the code really simple and less prone to errors.

[Hilderin]

I finally created a _extract_error_message_xml that does exactly what you suggest and take only the text nodes.
Example of an error message:

[demolke]

Thanks

[akien-mga]

Thanks @Hilderin for the fix and everyone else for the in-depth review!