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

VideoPlayer: Fix reloading translation remapped stream #84794

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Nov 12, 2023

MRP ported from Godot 3 (might need opening a couple of times, there's some weirdness with the font):
VideoRemaps.zip

Notes

  • For some reason, .ogv extension doesn't show up in the list of resources that can be picked from translation remaps. So one needs to select "All Files" to select .ogv files.
  • A good UX improvement would be to parse the end of the file to see if it's a locale name, and automatically pre-select the matching locale (and country if relevant), so video_de.ogv should automatically select "German", and video_de_AT should automatically select "German (Austria)".
  • This changes means that if the VideoStream resource changes while it's being played, the playback will stop. It's still possible for users to handle automatically restarting the playback when that happens, with something like this (modified from MRP):
extends Control

var is_playing := false

func _ready() -> void:
	$Video.stream.changed.connect(_on_stream_changed, CONNECT_DEFERRED)
	_on_EN_pressed()

func _on_stream_changed() -> void:
	if is_playing:
		_on_Play_pressed()

func _on_video_finished():
	is_playing = false

func _on_EN_pressed() -> void:
	TranslationServer.set_locale("en")

func _on_DE_pressed() -> void:
	TranslationServer.set_locale("de")

func _on_RU_pressed() -> void:
	TranslationServer.set_locale("ru")

func _on_Play_pressed() -> void:
	print("pressed")
	$Video.stop()
	$Video.play()
	is_playing = true

But it may not be easy to seek to the previous position. To do so, we would need to only emit changed, but not handle in the VideoStreamPlayer, leaving it to the user to implement.

Alternatively the VideoStreamPlayer could have more signals like stream_changed that could include relevant information about the previous state (was playing, position, etc.). But the current player is fairly barebones and that's way outside the scope of this bugfix.

@akien-mga akien-mga added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:gui cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Nov 12, 2023
@akien-mga akien-mga added this to the 4.3 milestone Nov 12, 2023
@akien-mga akien-mga requested review from a team as code owners November 12, 2023 14:40
@KoBeWi
Copy link
Member

KoBeWi commented Nov 12, 2023

For some reason, .ogv extension doesn't show up in the list of resources that can be picked from translation remaps. So one needs to select "All Files" to select .ogv files.

I investigated and it's caused by this:

bool ResourceFormatLoaderTheora::handles_type(const String &p_type) const {
return ClassDB::is_parent_class(p_type, "VideoStream");
}

The best part is that any resource with this method will not appear on the list. Consequently, any resource you can select does not implement handles_type(). I wonder what is this method for 🙃

@@ -237,6 +237,12 @@ bool VideoStreamPlayer::has_loop() const {
void VideoStreamPlayer::set_stream(const Ref<VideoStream> &p_stream) {
stop();

// Make sure to handle stream changes seamlessly, e.g. when done via
// translation remapping.
Copy link
Member

@KoBeWi KoBeWi Nov 12, 2023

Choose a reason for hiding this comment

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

Shouldn't this comment be below, at connect()?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was meant to cover both at once, but since they're split it's not very clear yeah.

@akien-mga akien-mga merged commit f444818 into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga akien-mga deleted the videoplayer-fix-reloading-translation-remapped-stream branch December 4, 2023 22:09
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4. (Adjusted for the old API lacking connect_changed/disconnect_changed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VideoPlayer: Remaps not supported
3 participants