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

Enable script class resource exports. #32018

Closed
wants to merge 1 commit into from

Conversation

willnationsdev
Copy link
Contributor

@willnationsdev willnationsdev commented Sep 6, 2019

Closes godotengine/godot-proposals#18 for GDScript. To add support for custom resource exports in other languages, all you would have to do is add script class support to those languages (godotengine/godot-proposals#22) and program their export code to correctly set the class_name and hint_string values for the PropertyInfo object.

Credits:

Much of this work derives from @vixelz's efforts from #26162, so some credit is due there.

Changes:

I have fixed some of the concerns addressed by those who commented on that PR, namely...

  • regular export hints getting messed up in the process (fixed, screenshot demonstration below)
  • it operating from scanning an identifier rather than reducing a constant (it now reduces a constant and iterates through languages to check if any of them can find a script class name for the script).

Caveats:

As for using preloaded constants, I have decided to forego any implementation on purpose since...

  1. Preloaded constants only have a name within the scope of the actual script they reside in. When the editor views the name, there is no globally recognizable location in which it can pull the name and convert it into a script instance to test if the assigned resource instance is type-constrained properly.
  2. Non-integer constants are not a concept within the Object API so they cannot be language-agnostically accessed from editor code even if it did know the name of the constant. This point is pretty ironic too since most scripting languages implement their own support for read-only StringName->Variant maps to enable the use of constants anyway. Maybe if that feature became part of the Object API, we would be able to add better support for this as it would be "just another place to check" when examining where a valid inheritor comes from.

Screenshots:

Here are some images of the PR in action. It demonstrates...

  1. Various export hints combined with type hints all working together.
  2. Using a MyResource export hint and assigning to it a DerivedMyResource instance.
  3. The resource dropdown displays the list of types that correctly extend the custom resource script class.
  4. What it looks like when you preload a constant and attempt to export it (you get an error).

example_node_script

example_export_derived_in_dropdown

bad_script_class_export

Edit: Fixed a bug where the custom resource icons were not displaying in the Inspector. The screenshots are outdated in this respect, so keep in mind that class icons will appear in the resource dropdown menu.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Sep 7, 2019

I have discovered a bug thus far where the custom resources do not match up when using MyResource for both the export hint and the type hint. Working on fixing it now.

Edit: bugs fixed! Here are the results of the export hint, type hint, and expression type combinations:

# works because null is a valid MyResource value
# works because DerivedMyResource is a valid MyResource value
export(DerivedMyResource) var my_resource: MyResource = null

# works because the expression on the right-hand side is a valid MyResource value
export(DerivedMyResource) var my_resource: MyResource = MyResource.new()

# does NOT work because not all possible MyResource values are valid DerivedMyResource values!
export(MyResource) var my_resource: DerivedMyResource = null

@ghost
Copy link

ghost commented Sep 9, 2019

Hey Will, thank you so much for this! I tested it out and noticed that while the format export(MyRes) var res worked fine, it doesn't seem to like export var res: MyRes at all. Would it be possible to support both?

image

@willnationsdev
Copy link
Contributor Author

Ah, that's a new error. Haven't tried omitting the export type and inferring it from the type hint. Will take a look at that tomorrow.

@willnationsdev
Copy link
Contributor Author

@somnivore This should now be fixed. Please re-test things and let me know if you encounter any issues.

@rsousacode
Copy link

rsousacode commented Sep 20, 2019

Was this tested in C#? Apparently i cannot manage to work

// also tested omiting Resource
 class MyClass : Resource
 {
     [Export]string text1;
     [Export]string text2;
 }

[Export] private readonly MyClass _myClass;

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Sep 20, 2019

This will not work for C# untill script class support is added for it (godotengine/godot-proposals#22). I am actually currently working on that, hehe.

@rand0m-cloud
Copy link

Commit 1fe8ded causes a merge conflict for your branch. I believe I can resolve it though.

@rand0m-cloud
Copy link

I believe this is the proper fix rand0m-cloud@952e991
1fe8ded replaced tokenizer->advance(); with a macro, so I replaced it in your commit with the marco.

@willnationsdev
Copy link
Contributor Author

@winadam Thanks for letting me know. I'll push an update to the PR soon.

@xDGameStudios
Copy link
Contributor

xDGameStudios commented Feb 18, 2020

Will we have the luck of seeing this in v4? along side project wide groups?

@willnationsdev
Copy link
Contributor Author

@xDGameStudios I'm not sure what you mean by resource groups (there a PR about that I missed?), but yes, I believe this will get merged for the 4.0 release at some point. Probably after all the major core revisions are completed and the master branch becomes slightly more stable.

@Reneator
Copy link

Reneator commented Mar 4, 2020

Edit2: I solved this problem by moving over to the 3.2 stable branch and the only changes i had to do was replace the export hints with ''Resource'' instead of my custom Ressource class. The defined Resources still remained and im able to successfully export again. (If somebody else might happen to be in the same predicament as me)

Hi,

Im using these changes in my project, but im unable to compile working export-templates for any platform. The compilation for those export templates works, but the compiled game doesnt work (crash on windows, "'' is not a valid selector" on html). If i export the project with the 3.2 stable export template, the game works, but is missing all the custom resources.

im currently using thise branch for my project, as the changes in this pull request are making things a lot easier for me (defining data).

Im currently working on a locally compiled version of (https://github.com/Reneator/godot/commits/master_2020).

Is there a plan/date to get this pull-request into master in the nearby future? or is there anything i can do, to help? (no C/C++ knowledge)

Edit: Changed formulation of last paragraph

@Arrow-x
Copy link

Arrow-x commented Apr 5, 2020

this should be picked for 3.2, it makes working with resources so powerful

@astrale-sharp
Copy link

Hey, i cloned your repo and tried to build the gdres branch :

$ scons -j8 platform=x11 use_llvm=yes use_lld=yes

[...] #compile fines
[ 38%] Compiling ==> modules/gdnative/pluginscript/pluginscript_script.cpp
[ 38%] Compiling ==> modules/gdnative/pluginscript/register_types.cpp
modules/gdnative/arvr/arvr_interface_gdnative.cpp:290:2: warning: this needs to
      be redone [-W#warnings]
#warning this needs to be redone
 ^
modules/gdnative/arvr/arvr_interface_gdnative.cpp:280:7: warning: unused
      variable 'render_target' [-Wunused-variable]
        RID *render_target = (RID *)p_render_target;
             ^
modules/gdnative/arvr/arvr_interface_gdnative.cpp:304:2: warning: need to obtain
      this ID again [-W#warnings]
#warning need to obtain this ID again
 ^
modules/gdnative/arvr/arvr_interface_gdnative.cpp:302:6: warning: unused
      variable 'eye_texture' [-Wunused-variable]
        RID eye_texture = VSG::storage->render_target_get_texture(*rende...
            ^
[ 38%] Compiling ==> modules/gdnative/videodecoder/register_types.cpp
[ 38%] Compiling ==> modules/gdnative/videodecoder/video_stream_gdnative.cpp
[ 38%] Compiling ==> modules/gdnative/gdnative_api_struct.gen.cpp
4 warnings generated.
[ 38%] Compiling ==> modules/freetype/register_types.cpp
[ 38%] Compiling ==> thirdparty/etc2comp/EtcBlock4x4.cpp
[ 38%] Linking Static Library ==> modules/libmodule_freetype.x11.tools.64.llvm.a
Ranlib Library         ==> modules/libmodule_freetype.x11.tools.64.llvm.a
modules/gdnative/gdnative_api_struct.gen.cpp:60:2: error: cannot initialize a
      member subobject of type 'void (*)(int, godot_rid *, godot_rect2 *)' with
      an lvalue of type 'void (godot_int, godot_rid *, godot_rect2 *)
      __attribute__((sysv_abi))'
      (aka 'void (long, godot_rid *, godot_rect2 *)'): type mismatch at 1st
      parameter ('int' vs 'godot_int' (aka 'long'))
        godot_arvr_blit,
        ^~~~~~~~~~~~~~~
1 error generated.
[ 38%] Compiling ==> thirdparty/etc2comp/EtcBlock4x4Encoding.cpp
scons: *** [modules/gdnative/gdnative_api_struct.gen.x11.tools.64.llvm.o] Error 1
scons: building terminated because of errors.


@willnationsdev
Copy link
Contributor Author

@astrale-sharp That is probably because it's in the middle of trying to get converted into a 4.0-compatible build. I have a separate gdres-3.2 branch that takes the current 3.2 branch from godotengine/godot and cherry-picks this PR's single commit onto it. Should compile with no issues, afaik.

@willnationsdev
Copy link
Contributor Author

Rebased

@RandomShaper
Copy link
Member

@willnationsdev, I can't find your gdres-3.2. Is there still some chance to find it somewhere?

@Reneator
Copy link

@willnationsdev, I can't find your gdres-3.2. Is there still some chance to find it somewhere?

you can find it in his personal forked godot repository:

https://github.com/willnationsdev/godot/tree/gdres-3.2

when i looked for it in branches, i couldnt find it, but in the Active category it still exists.

@willnationsdev
Copy link
Contributor Author

@RandomShaper @Reneator I'm not sure why it wouldn't show up for you. If I just go to my repo and click the branch dropdown, it shows up for me.

@MeikelLP
Copy link

@willnationsdev what's the status of this? I'd love to see this in 3.2.4 :)

@willnationsdev
Copy link
Contributor Author

@MeikelLP I had a port of this for 3.2 in my gdres-3.2 branch. @JFonS updated it to a recent build of 3.2 and has submitted a separate pull request for it (#41264). I actually just recently made a whole bunch of local improvements to this PR (currently in my gdres_gd2 branch - some of which may benefit JFonS's work if he wanted to grab it, drag-n-drop stuff, etc.) and was going to have it fully updated with GDScript 2.0, but the new parser doesn't yet have any validation of data types for export, and as a result, cannot reasonably export resources, let alone custom ones (#42023). So, until that stuff is resolved, this PR is at a standstill. But there's a decent chance support for this will come in 3.2.4, by my guess.

@willnationsdev
Copy link
Contributor Author

Just an update for folks here. I re-tested the baseline of features in 4.0 to see if I could even properly test whether this PR's code works, and there are still some issues preventing me from doing so (which means there's no point in even updating the PR's code yet). Unfortunately, because exporting basic resources still doesn't work yet, I can't update this PR. See #43904 for details.

@willnationsdev
Copy link
Contributor Author

A full 3.2 port of this which includes not only GDScript support, but also VisualScript, NativeScript, PluginScript, and C# can now be found at #44879.

After GDNative 2.0 is implemented, I'll have to refactor this PR to do all the same features as the 3.2 port, but building off of the new core foundation for script classes.

@Sslaxx
Copy link

Sslaxx commented Jun 21, 2021

After GDNative 2.0 is implemented, I'll have to refactor this PR to do all the same features as the 3.2 port, but building off of the new core foundation for script classes.

Any plans for this?

@willnationsdev
Copy link
Contributor Author

@Sslaxx This PR is outdated. For updated builds, see #48201 for the 4.0 version and #44879 for the 3.x version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add first-class custom resource support