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

#[export(file)] should be limited to certain field types #772

Open
snatvb opened this issue Jun 19, 2024 · 9 comments
Open

#[export(file)] should be limited to certain field types #772

snatvb opened this issue Jun 19, 2024 · 9 comments
Labels
bug c: register Register classes, functions and other symbols to GDScript

Comments

@snatvb
Copy link

snatvb commented Jun 19, 2024

If user tries convert @export_file("*.txt") to Array:

extends Node

class_name NewScript

@export_file("*.txt") var files: Array[String]

LSP shows error:
image
And in console:
image

But if user does it in rust:

#[derive(GodotClass)]
#[class(init, base=Node)]
struct Foo {
    base: Base<Node>,
    #[export(file)]
    resources: Array<GString>,
}

#[godot_api]
impl INode for Foo {}

It leads to this:
image
image

For user it's unexpected and it would be rather to notify about incorrect annotation.

Reprocase: godot-rust-repro.zip

Discord source conversation

@snatvb
Copy link
Author

snatvb commented Jun 19, 2024

godotengine/godot#93366
Created on Godot, if there is no point in improving the macro, you can close it down

@lilizoey
Copy link
Member

godotengine/godot#93366 Created on Godot, if there is no point in improving the macro, you can close it down

i dont see why this issue should be brought up to godot

@lilizoey lilizoey added bug c: register Register classes, functions and other symbols to GDScript labels Jun 19, 2024
@snatvb
Copy link
Author

snatvb commented Jun 19, 2024

i dont see why this issue should be brought up to godot

Bromeon said that it's issue of Godot that it doesn't notify about incorrect export property

@Bromeon
Copy link
Member

Bromeon commented Jun 19, 2024

Sorry about the confusion here, tried to clarify it on the Godot issue.

I'm not sure whether Godot performs the same validations for @export_* annotations in GDScript as it does for GDExtension property hints. If it doesn't (and there is no change planned), then we would need to replicate them on Rust side, which could be quite laborious and maintenance-intensive.

@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

@export_file on Array

Since GDScript doesn't support this annotation to be used on Array[String], we should probably follow and provide a compile-time (or startup) error.


Error message

From dsnopek's comment on the upstream issue:

I suppose we could put similar validation in ClassDB::add_property(), however, the error message couldn't really be as good.

From Godot's perspective, it's looking at data passed on the PropertyInfo struct, and the error message could only refer to that (for example, saying the PropertyInfo::hint won't work with the given PropertyInfo::type). However, the binding probably doesn't directly expose PropertyInfo, but instead some binding-specific way of doing it, and so it'd be able to provide a better error message that refers to that.

So, I personally think giving a nice error message should be up to the GDExtension binding (in this case, the Rust binding).

TLDR there's limited things that can be done on Godot side, and a more specific error message on our side (tailored to Rust APIs) probably makes sense.

@snatvb
Copy link
Author

snatvb commented Jul 8, 2024

Won't it support in 4.3? I've seen some information with improvement this part. Maybe I'm wrong, can't find anymore :(

@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

I don't know -- do you have the time to test your example with a recent 4.3 beta release?

@snatvb
Copy link
Author

snatvb commented Jul 11, 2024

not really, it requires build
it's still a little difficult for me
you can hold the task until 4.3 is released

@Bromeon
Copy link
Member

Bromeon commented Jul 11, 2024

No need to build -- you can get nightly artifacts from https://github.com/Bromeon/godot4-nightly/actions 🙂

Just click on the latest workflow run, scroll down and select the matching artifact for your platform (e.g. godot-windows-nightly).

@Bromeon Bromeon changed the title Improvement expected behaviour for #[export] #[export(file)] should be limited to certain field types Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: register Register classes, functions and other symbols to GDScript
Projects
None yet
Development

No branches or pull requests

3 participants