godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix `Find in Files` Search Results cannot open builtin script

Open stmSi opened this issue 1 year ago • 3 comments

Fix #74145
I don't think this is best way to do this but this is all I can come up with. Suggestions for better way are welcome.

stmSi avatar Mar 04 '23 22:03 stmSi

Yeah this is not a correct way to do this.

  1. You are instantiating the scene, which is rather overkill. And unreliable, because the script can be inside e.g. exported property or attached to a resource
  2. You are not freeing the nodes afterwards (this is easy to fix tho)
  3. It doesn't seem to work correctly with multiple scripts in scene. I clicked a result, but it opened me wrong script
  4. You still need to open the scene when you open the script. Built-in resources can't be edited outside their scene, because any changes you do won't be saved

The solution isn't really bad, but perfectly it should work without creating the nodes. I don't know if there is any way to load the resources from PackedScene without instantiating it.

However, I once opened a proposal for the same problem: https://github.com/godotengine/godot-proposals/issues/6032 My idea was that built-in GDScripts could be mapped to lines in the file and cached, but tbh I'm not sure where to put that cache. Maybe loading the scene each time is fine, but instead of loading as resource, open it as text instead and manually find the line?

How it could potentially work:

  1. User searches for "something"
  2. Editor finds it in line 79 of a tscn
  3. User clicks that line
  4. Editor opens the tscn as a text file
  5. It travels the file line-by-line and looks for GDScript sub-resource headers
  6. If it finds one, it parallelly counts the current line in the script a. If it reaches line 79, it opens the scene + script on the counted script line b. If it reaches end of the script (line ends with unescaped "), it looks for the next script until line 79

KoBeWi avatar Mar 06 '23 12:03 KoBeWi

@KoBeWi I followed your suggestion and rewrite the logic. Can you check it again?

You still need to open the scene when you open the script. Built-in resources can't be edited outside their scene, because any changes you do won't be saved

I could not open both the scene and the script. EditorNode::get_singleton()->load_scene(fpath); and ste->goto_line_selection don't want to work together. First time clicking will open the scene. And Second Click will open the script file

stmSi avatar Mar 06 '23 17:03 stmSi

EditorNode::get_singleton()->load_scene(fpath); and ste->goto_line_selection don't want to work together.

Pretty sure this can be solved with call_deferred().

KoBeWi avatar Mar 06 '23 17:03 KoBeWi

tested ste->call_deferred(SNAME("goto_line_selection"), line_number, begin, end); and it doesn't seem to work. Scene opened. Builtin Script opened. But Line Selection doesn't work. Caret is at the top.

So I changed back to normal and tested EditorNode::get_singleton()->call_deferred(SNAME("load_scene"), fpath) it doesn't seem to work either.

stmSi avatar Mar 06 '23 19:03 stmSi

idk I tested and it works, but the editor does not center on the selected line. EDIT: External scripts are not centered either, so probably fine like this for now.

KoBeWi avatar Mar 06 '23 19:03 KoBeWi

I checked the code and it looks fine. I found one minor problem, other suggestions are just comment style fixes.

KoBeWi avatar Mar 06 '23 20:03 KoBeWi

Thanks for suggestions. Updated.

stmSi avatar Mar 06 '23 20:03 stmSi

Can't we use ResourceFormatLoaderText to parse the tscn and extract the GDScript subresource instead of reimplementing an ad hoc parser?

Edit: Note that I'm not sure this loader specifically is suitable, it just looks to me like we're reimplementing stuff which Godot can already do. But I see previous discussion seemed to propose doing this intentionally.

akien-mga avatar Mar 07 '23 07:03 akien-mga

I will look into it now.

stmSi avatar Mar 07 '23 07:03 stmSi

Using ResourceFormatLoaderText involves parsing the whole file. The current approach is much faster.

KoBeWi avatar Mar 07 '23 10:03 KoBeWi

I am experimenting VariantParser::parse_tag_assign_eof approach


			while(true){

				String assign;
				Variant value;

				Error error = VariantParser::parse_tag_assign_eof(&stream, lines, error_text, next_tag, assign, value, nullptr);
				if(error == Error::ERR_FILE_EOF){
					break;
				}

				if (next_tag.name != "sub_resource") {
					continue;
				}
				if(!next_tag.fields.has("type")){
					continue;
				}

				print_line("TYPE");
				print_line(next_tag.fields["type"]);

				if(next_tag.fields["type"] != "GDScript"){
					continue;
				}

				if(!next_tag.fields.has("id")){
					continue;
				}
				scr_id = next_tag.fields["id"];

It can work though. But if statements are the same as using [sub_resource type=\"GDScript\" id=\"

stmSi avatar Mar 07 '23 10:03 stmSi

Let's go with the current approach then :)

akien-mga avatar Mar 07 '23 11:03 akien-mga

Thanks!

akien-mga avatar Mar 07 '23 11:03 akien-mga

Cherry-picked for 4.0.2.

YuriSizov avatar Mar 27 '23 15:03 YuriSizov