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

Expose OS.read_string_from_stdin() to the scripting API (3.x) #70378

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Dec 20, 2022

3.x version of #65751. See godotengine/godot-proposals#2322.

This can be used in scripts to read user input in a blocking manner.

This also removes the unused block argument, which is always true.

# Save this script as `example.gd`, then run:
#     godot -s my_script.gd
extends SceneTree

func _initialize():
	var text = ""
	while text != "quit":
		printraw("Insert some text: ")
		text = OS.read_string_from_stdin().strip_edges()
		print("You inserted: %s" % text)
	print("Quitting")
	call_deferred("quit")

Note that non-ASCII input doesn't appear to be handled correctly (tested on Linux):

Insert some text: 123 Hello × é
You inserted: 123 Hello
Insert some text: ééé×××x×
You inserted: x

In master, this occurs instead:

Insert some text: 123 Hello × é
You inserted: 123 Hello à é
Insert some text: ééé×××x×
You inserted: éééÃÃÃxÃ

Maybe @bruvzg knows about resolving this encoding issue?

@bruvzg
Copy link
Member

bruvzg commented Dec 21, 2022

Maybe @bruvzg knows about resolving this encoding issue?

Decoding is missing completely, so it's reading it as Latin-1. Fix - #70389

@@ -520,6 +520,10 @@ Error _OS::shell_open(String p_uri) {
return OS::get_singleton()->shell_open(p_uri);
};

String _OS::read_string_from_stdin(bool p_block) {
return OS::get_singleton()->get_stdin_string(true);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return OS::get_singleton()->get_stdin_string(true);
return OS::get_singleton()->get_stdin_string(p_block);

But probably the argument should be removed at all (from both CoreBind and OS), it's not useful, setting it to false always return empty string.

Copy link
Member Author

@Calinou Calinou Dec 29, 2022

Choose a reason for hiding this comment

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

The argument was likely kept in #65751 for forward compatibility (in case non-blocking stdin reading is implemented), but it's not likely to happen in 3.x.

Copy link
Member

Choose a reason for hiding this comment

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

I think the argument should be removed here and for 4.0 too. If it's not implemented, there's no point having it. It can be added later if we do implement it.

Copy link
Member

Choose a reason for hiding this comment

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

I would also remove the unused parameter from the OS method, not just the binding.

@Calinou Calinou force-pushed the os-expose-read-from-stdin-3.x branch from b835626 to 422818d Compare December 29, 2022 20:03
@Calinou Calinou changed the title Expose OS.read_string_from_stdin() to the scripting API Expose OS.read_string_from_stdin() to the scripting API (3.x) Dec 29, 2022
@Calinou Calinou force-pushed the os-expose-read-from-stdin-3.x branch 6 times, most recently from c8beeb6 to cf603aa Compare January 11, 2023 18:54
@Calinou Calinou requested review from a team as code owners January 11, 2023 18:54
@Calinou
Copy link
Member Author

Calinou commented Jan 11, 2023

I've noticed that stdin length is limited to 1024 bytes:

https://github.com/godotengine/godot/blob/cf603aae8b7de7dd91066003b9898a97373eeed8/platform/windows/os_windows.cpp#L3258-L3259

https://github.com/godotengine/godot/blob/cf603aae8b7de7dd91066003b9898a97373eeed8/drivers/unix/os_unix.cpp#L140-L143

The same limitation is also present in master.

Is there a way we could increase this without side effects? If not, I can document it. cc @bruvzg

Edit: This is being addressed by #88863.

@bruvzg
Copy link
Member

bruvzg commented Jan 11, 2023

I've noticed that stdin length is limited to 1024 bytes:

I guess we can call read in a loop and add to String, like

	char buff[1024];
	String ret;
	while (fgets(buf, 1024, stdin) != nullptr) {
		ret += buf;
	}
	return ret;

Also, will need #70389 backported for Unicode support.

doc/classes/OS.xml Outdated Show resolved Hide resolved
@Calinou Calinou force-pushed the os-expose-read-from-stdin-3.x branch from cf603aa to 0c405b9 Compare January 16, 2023 09:17
This can be used in scripts to read user input in a blocking manner.

This also removes the unused `block` argument, which is always `true`.
@Calinou Calinou force-pushed the os-expose-read-from-stdin-3.x branch 2 times, most recently from 7ed1a59 to badcfa2 Compare January 16, 2023 10:39
@akien-mga akien-mga merged commit f3da393 into godotengine:3.x Jan 20, 2023
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the os-expose-read-from-stdin-3.x branch January 20, 2023 11:43
@Calinou
Copy link
Member Author

Calinou commented Jan 30, 2023

I guess we can call read in a loop and add to String, like

	char buff[1024];
	String ret;
	while (fgets(buf, 1024, stdin) != nullptr) {
		ret += buf;
	}
	return ret;

I've tested this locally and it appears to freeze after entering a long string.

The current behavior is that calling OS.read_stdin_from_string() multiple times will do the concatenation for you. Also, if you enter a string before calling OS.read_stdin_from_string(), it will be read immediately without blocking.

Testing project: test_long_stdin.zip

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.

3 participants