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

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

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

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.

@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.

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

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

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

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).

@snatvb so it looks like we would need to add validation on godot-rust side.

I have an idea how to validate this in a lightweight way. To be clear, I don't want to make the type-system more complex by adding niche traits, or worse, handling at the proc-macro level. I'm very fine with having this validation during startup rather than at compile time.

The export_file implementation currently looks like this:

pub mod export_info_functions {
    pub fn export_file<S: AsRef<str>>(filter: S) -> PropertyHintInfo {
        ...
    }
}

with PropertyHintInfo (this one shouldn't be changed, as it's used in other places):

#[derive(Clone, Eq, PartialEq, Debug)]
pub struct PropertyHintInfo {
    pub hint: PropertyHint,
    pub hint_string: GString,
}

Now to support this, all export functions could additionally (in a tuple) return a validator function, which would be |_| true by default.

type ValidateExportType = Box<dyn Fn(&PropertyInfo) -> bool>;
// or:
type RestrictExportType = Option<Box<dyn Fn(&PropertyInfo) -> bool>>;

The PropertyInfo passed to it can be obtained as <<FieldType as Var>::Via as GodotType>::property_info():

In this case for example, we could then return:

|info| {
    info.variant_type == VariantType::STRING // maybe STRING_NAME and NODE_PATH are OK, too
}

Should be relatively straightforward to implement.

@Bromeon as I remember there is issue that I have no option to add file-type for array. If I want to use several resources and take file path - I can't do it. For example it could be textures. When I use @export_file("*.txt") - it gives me not regular text input, it gives me file input control and I can limit extensions.

@Bromeon as I remember there is issue that I have no option to add file-type for array.

This is not supported in Godot, see docs and error message in your issue godotengine/godot#93366. So my proposal is to explicitly disallow this in godot-rust, too, in order to provide a meaningful error message.

If you want this feature, you'd need to lobby further for it in godotengine/godot#93366 (or maybe create a new one, given that the discussion was now mostly about the GDExtension error capability).

@Bromeon as I remember there is issue that I have no option to add file-type for array.

This is not supported in Godot, see docs and error message in your issue godotengine/godot#93366. So my proposal is to explicitly disallow this in godot-rust, too, in order to provide a meaningful error message.

If you want this feature, you'd need to lobby further for it in godotengine/godot#93366 (or maybe create a new one, given that the discussion was now mostly about the GDExtension error capability).

Not really, I've re-checked the repro case and what I have (use NewScript node):

extends Node

class_name NewScript

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

And what I have rust version node:

Sorry I forget and NewScript node to present difference

UPD: ah, I think I got it. It doesn't support for GDExentions, right?

I went ahead and implemented this in #1166.
@snatvb could you test if that works as expected?

I'll check it out the other day, thanks

I went ahead and implemented this in #1166. @snatvb could you test if that works as expected?

So, I've updated lib version to 0.2.4.
Used this code:

use godot::prelude::*;

struct RustExtension;

#[gdextension]
unsafe impl ExtensionLibrary for RustExtension {}

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

#[godot_api]
impl INode for Foo {}

Godot: 4.4.1 and still I see the same issue. But I see your code in PR and can't understand why it doesn't work 🧐

UPD: I've added this:

    #[export(file = "*.txt")]
    export_file_wildcard_parray: PackedStringArray,

And got this:

So, I've updated lib version to 0.2.4.

You should check the pull request I linked, not version 0.2.4.
The changes I made aren't integrated to master yet, let alone released on crates.io.

To do this, update the Cargo.toml dependency as follows:

[dependencies]
godot = { git = "https://github.com/godot-rust/gdext", branch = "feature/export-file-arrays" }

So, I've updated lib version to 0.2.4.

You should check the pull request I linked, not version 0.2.4. The changes I made aren't integrated to master yet, let alone released on crates.io.

To do this, update the Cargo.toml dependency as follows:

[dependencies]
godot = { git = "https://github.com/godot-rust/gdext", branch = "feature/export-file-arrays" }

oh, my bad, my bad. I've thought that it's already merged. I apologize.
So, I can approve that it works fine on this case:

I'll leave it open, and I think you can close it when #1166 is merged :)

Many thanks for your work! 🚀✨

Thanks a lot for the testing! 👍

Yes, I'll likely merge it for v0.3.1, and close this issue then.
If you need the feature before, just keep using that branch until then 🙂