Reviewing a StoryQuest submission

The StoryQuest-submission PRs can be massive, and the GitHub website may not handle it. So here are commands to review the submission locally and help learners with their PRs.

Local checkout

To checkout a PR locally, the best way is to do it using the gh command (specific to GitHub):

gh pr checkout Game-Lab-5-0-UTP-Group-4-Team-4:main

Although it is possible to do it using Git only:

git fetch origin pull/946/head:nocturnos-main
git switch nocturnos-main

Where 946 is the PR number and nocturnos-main is the local name that it will have.. So this will fetch #946 .

Now nocturnos-main is a branch tracking the PR (most likely the fork's main), and we can compare it with upstream main.

Checking for files changed

Use the triple dot to mimic what the "Files Changed" tab in GitHub shows:

git diff main...HEAD --stat

# Or directly:
git diff main... --stat

Here is a good explanation (with graphs included) for the triple-dot syntax in Git.

To find if the PR modifies any files in upstream main, --diff-filter=M can be used:

git diff main... --stat --diff-filter=M

This would spot, for instance, if project.godot was modified in the PR.

But more accurately, what you probably want for a storyquest submission PR is to check that it only contains new files added to a directory inside scenes/quests/story_quests/. So the --diff-filter=a would exclude added files, the output should be empty:

git diff main... --stat --diff-filter=a

To check specifically what learners changed in a file outside their storyquest directory:

git diff main... project.godot

To revert changes from a file outside their storyquest directory:

git restore --source=main... scenes/tileset.tres

If a StoryQuest has a script declaring a class_name, it should be prefixed. Check the following output:

git grep class_name scenes/quests/story_quests/shjourney/

If a scene added in the PR has zero owners, it can be removed from it.

You can check from the Filesystem Dock by selecting "View Owners..." from the right-click dropdown. But since the paths from that window can't by copy-pasted, you may want to use grep from CLI. First find the UID of the resource (same right-click dropdown, option "Copy UID") then use the part after "uid://" like this:

# Find all references in the project:
git grep iu2q66clupc6

# Find all references in a StoryQuest directory:
git grep iu2q66clupc6 scenes/quests/story_quests/shjourney/

Checking if files outside of the StoryQuest directory have been changed:

git diff main... --stat ':!scenes/quests/story_quests/shjourney'

Pushing changes to a contributed pull request

You can checkout a pull request using the gh command:

gh pr checkout gimms11-fix:main

We maintainers should be able to push directly to a pull request as stated in GitHub with the message "Maintainers are allowed to edit this pull request", if the submitter enabled that in their fork. However, due to an undocummented limitation it doesn't work with Git LFS unless you pass --no-verify:

$ git push git@github.com:Gimms11/threadbare.git +origin/gimms11-fix:main
error: Authentication error: Authentication required: You must have push access to verify locks
error: failed to push some refs to 'github.com:Gimms11/threadbare.git'

$ git push --no-verify git@github.com:Gimms11/threadbare.git +origin/gimms11-fix:main
warning: not sending a push certificate since the receiving end does not support --signed push
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
To github.com:Gimms11/threadbare.git
 + c74fffa2...a0ddb2fd origin/gimms11-fix -> main (forced update)

Checking for unneeded files

Learners may commit extra files than the ones actually used in the StoryQuest. Within the Godot Editor you can check for orphans using Project -> Tools -> Orphan Resource Explorer. The problem with that tool is that there might be false positives, for resources not explicitely referenced. For example, for tscn files that are referenced through the next_scene property of the Cinematic node, which is how we chain scenes. But it should work fine for assets like PNG or WAV files. Except that it is not easy to copy the paths from that popup dialog. So here is a rudimentary EditorScript that tries to do that. Very rudimentary, it should be improved!

@tool
extends EditorScript


func is_orphan(path: String) -> bool:
	var uid_str := ResourceUID.path_to_uid(path)
	var output := []
	var exit_code := OS.execute("/usr/bin/git", ["grep", uid_str, ":!*.import", ":!*.uid"], output)
	return exit_code == 1 and output.size() == 1 and output[0] == ""


func find_files(base_path: String) -> PackedStringArray:
	var found_files: PackedStringArray
	for item_name in ResourceLoader.list_directory(base_path):
		var sub_path := base_path.path_join(item_name)
		if item_name.ends_with("/"):
			found_files.append_array(find_files(sub_path))
		else:
			found_files.append(sub_path)
	return found_files


func _run() -> void:
	print_rich("[b]Assets without an explicit ownership[/b]")
	for path: String in find_files("res:///scenes/quests/story_quests/shjourney/"):
		if path.get_extension().to_lower() not in ["png", "wav", "mp3", "ogg"]:
			continue
		if is_orphan(path):
			print(path)

Also is worth checking for duplicated assets. Some dups are OK, and the automation to create StoryQuests ends up with duplicated assets. In Linux, the fdupes command line tool is helpful:

$ fdupes -r scenes/
scenes/game_elements/props/decoration/bush/components/Bush_Green_Small.png
scenes/quests/story_quests/renya_beyond_sorrow/1_moscas/bush/components/Bush_Green_Small.png

scenes/game_elements/props/decoration/bush/components/Bush_Purple_Small.png
scenes/quests/story_quests/renya_beyond_sorrow/1_moscas/bush/components/Bush_Purple_Small.png

(...)

Checking for big files

If possible, large asset files (WAVs, PNGs) should be compressed. This command will output the 9 biggest files in the StoryQuest:

$ find scenes/quests/story_quests/after_the_tremor  -type f -print0 | xargs -0 du -shc | sort -h | tail -n 10
428K	scenes/quests/story_quests/after_the_tremor/music/sounds/soccerkick.wav
992K	scenes/quests/story_quests/after_the_tremor/music/sounds/tremor.wav
1,9M	scenes/quests/story_quests/after_the_tremor/music/enteaparece.ogg
2,4M	scenes/quests/story_quests/after_the_tremor/3_combat/2_cinematicacombate/tile/magecity.png
5,6M	scenes/quests/story_quests/after_the_tremor/music/sounds/beach.wav
15M	scenes/quests/story_quests/after_the_tremor/music/batallainicio.wav
19M	scenes/quests/story_quests/after_the_tremor/music/minijuego3.wav
31M	scenes/quests/story_quests/after_the_tremor/music/minijuego1.wav
55M	scenes/quests/story_quests/after_the_tremor/music/minijuego2.wav
132M	total

Adjust the last -n 10 argument of tail to check for more, if the smallest is bigger than 1 MB.

For PNGs, you can use Curtail:

For audio, is better to compress by converting from WAV to OGG. Here are good values for oggenc (brew install vorbis-tools):

oggenc \
-t 'minijuego1' \
scenes/quests/story_quests/after_the_tremor/music/minijuego1.wav

Giving credit to multiple learners

Add Co-authored-by: NAME <EMAIL> lines. For the email we can generate noreply addresses from Github. Use this command:

gh api  --jq '(.name // .login) + " <" + (.email // (.id | tostring) + "+" + .login + "@users.noreply.github.com") + ">"' /users/USERNAME

For example:

$ gh api  --jq '(.name // .login) + " <" + (.email // (.id | tostring) + "+" + .login + "@users.noreply.github.com") + ">"' /users/wjt
Will Thompson <will@willthompson.co.uk>
$ gh api  --jq '(.name // .login) + " <" + (.email // (.id | tostring) + "+" + .login + "@users.noreply.github.com") + ">"' /users/159adrian
159adrian <213216604+159adrian@users.noreply.github.com>