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

BlockCanvas: Implement drag & drop the file from resource FileSystem #305

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

starnight
Copy link
Contributor

Dragging the files from resource FileSystem shows the files' type as
"files". So, allow the "files" type in _can_drop_data() and handle it in
_drop_data(). Then, implement getting the files' resource paths as
Variable blocks with _drop_files() in detail.

https://phabricator.endlessm.com/T35712

@starnight
Copy link
Contributor Author

This depends on #296. So, set it as draft now.

@starnight starnight marked this pull request as draft November 5, 2024 08:32
@starnight
Copy link
Contributor Author

Here is the demo video:

Screencast.From.2024-11-05.16-28-04.mp4

Define the format "{const <parameter name>: <type>}" as constant
parameter value. And, it is a kind of in parameter value, but never
changes.

https://phabricator.endlessm.com/T35712
Have get_resource_block_definition() providing resource block's
definition. It uses constant parameter value with key "file_path" to
hold the resource file's path information.

https://phabricator.endlessm.com/T35712
… dock

Dragging the resources from the filesystem dock shows the object's type
as "files". So, allow the "files" type in _can_drop_data() and handle it
in _drop_data(). Then, implement getting the resource's file path as a
Variable block with _drop_files() in detail.

And, because Godot allows dragging multiple files at the same time, this
implemention parses the "files"' paths, then generates the corresponding
block definition with get_resource_block_definition() and sets default
"file_path" for each file's path.

Besides, to avoid the dropped blocks overlap with each others on the
canvas, each block biases its position a little bit automatically.

https://phabricator.endlessm.com/T35712
…ile blocks

Drag & drop resouce files as blocks into canvas has been implemented.
This commit focuses on serializing from the resource file blocks to AST
nodes and values.

The resource file's path is saved as in the arguments "file_path" feild.
So, pass the arguments, which may have "file_path" into
get_block_definition() to rebuild resource file's block definition.

https://phabricator.endlessm.com/T35712
@starnight starnight force-pushed the T35712-drag-drop-resource-file branch from d9f104a to 714c31f Compare November 20, 2024 06:05
@starnight starnight marked this pull request as ready for review November 20, 2024 06:06
@starnight
Copy link
Contributor Author

starnight commented Nov 20, 2024

Here is the latest demo video:

Screencast.From.2024-11-19.19-40-13.mp4

Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

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

I'm leaving some nits. Good work!

var block_def := BlockDefinition.new()

# Block Definition's name cannot work with '.'
block_def.name = &"get_resource_%s" % file_name.replace(".", "_")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what happens if the file name has a whitespace in it. Is file_name here already escaping it?

if not block_name.begins_with("get_resource_"):
return null

if file_path == "":
Copy link
Contributor

Choose a reason for hiding this comment

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

From docs:

is_empty should be used to check for empty strings.

Suggested change
if file_path == "":
if file_path.is_empty():

if file_path == "":
return null

var file_name = file_path.get_file()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance that this string doesn't have a valid file? Eg. what would happen if the user removes the file from the filesystem? In that case what does get_file() return?


for file_path in resource_files:
# Prepare a Variable block getting the file's resource path.
var block_definition = _res_file_block_def(file_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

The function name _res_file_block_def is not self-explanatory. Maybe _get_block_definition_from_resource_file()? Or at least _get_blockdef_from_resource_file()?

By the way, that function is using BlocksCatalog.get_resource_block_definition which is more clear. Maybe the helper function is not needed at all?

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.

2 participants