Add additional clarity around expanding the Stream resource in the inspector

[raziel444]

No description provided.

[tetrapod00]

Is something wrong with the way that the tutorial currently tells you to do it? It still works for me in 4.4beta3:

I looked on the page and found this comment chain, which ends with the suggestion to use Parameters. But I think that this user just didn't click on the Stream resource to expand it, which is needed to see the Loop property.

I think we could address that by making it clear that you have to click on the audio file in the inspector, where it says "OggVorbis", to expand the resource property.

[tetrapod00]

Suggested change

	select ``Make Unique``, then expand the Parameters group and check the
	``Looping`` box.
	select **Make Unique**, then expand the **Stream** resource in
	the inspector by clicking on ``OggVorbis`` and enable the **Loop**
	property.

Something like this to make it very explicit where to click. I kind of wish that the dropdown arrow menu had an Expand in Inspector option, as an alternative.

(Changing from code to bold for some of these is an unrelated style standardization that we might as well do now.)

[raziel444]

I think this will be a helpful addition, as this is what I struggled to understand.

[raziel444]

Is the below a reasonable way to describe this? At this point in the instruction the inspector will display either House In a Forest Loop.ogg or gameover.wav, rather than OggVorbis, so I am looking for something else to refer to.

All audio is automatically imported with the **Loop** setting disabled.
If you want the music to loop seamlessly, click on the Stream file arrow,
select **Make Unique**, then expand the **Stream** resource in
the inspector by clicking on the title of the sound file you selected 
(near the colorful music note) and enable the **Loop** property.

[tetrapod00]

In 4.4beta3, when I click Make Unique, the AudioStream does get renamed to OggVorbis. So IMO the suggestion I made already is fine, and mentioning the name of the sound file might be misleading since the visible name changes.

If we wanted to hedge against both scenarios I would do this:

All audio is automatically imported with the **Loop** setting disabled.
If you want the music to loop seamlessly, click on the Stream file arrow,
and select **Make Unique**. Then expand the **Stream** resource in
the inspector by clicking on the name of the stream, by the colorful
music note icon, and enable the **Loop** property.

I broke up the long winding sentence a bit, and used "name" rather than "title" to hedge against the case where it gets renamed to just "OggVorbis".

Note that the WAV has different properties than the OggVorbis and is much harder to set up properly to loop. I don't believe that the tutorial intends for you to enable looping on the WAV. So we don't need to account for that.

At this point we're nitpicking over details though, I think either your suggestion or either of mine would be fine

[raziel444]

Thank you - I like your version better. I'll get it updated later today.

[tetrapod00]

The text looks good now, just need to revert the image change since it's no longer needed

[raziel444]

I've created a new commit to restore the original image and incorporate suggestions from @tetrapod00, but I am unsure of best practices for the pull request.

Is it preferred to close this PR and open a new one that has a single commit so it's clear what was being modified?

[tetrapod00]

In most cases it's best not to make a new PR if you can avoid it, rather change the existing PR, updating the title and PR message to reflect the current changes.

Best practice would be to squash commits into one using interactive rebase, see our instructions here. This is required for the main Godot repo.

But in the docs repo, we also use GitHub's Squash and Merge workflow which allows us to merge PRs with multiple commits. So if the squash with interactive rebase still feels intimidating it's fine to leave multiple commits.

On my end I still see the new, changed image, not the old one that currently exists in the docs. Are you sure you changed it back?

[raziel444]

I am absolutely not sure that I have changed it back! Thanks for walking me through the process - I'm quite new to most of this. I will read through the contribution workflow since I was unaware it existed.

[tetrapod00]

Current state looks good to me. You can mark this as "ready for review". If you wanna practice squashing again feel free to rebase again 🙂

Another reviewer will take a look before this is merged

[raziel444]

Apologies, I was trying to get it down to a single commit with the image changes reverted, but I seem to be one step away each time.

[raziel444]

I'm unsure what occurred - it shows I closed the PR, but I hadn't intended to do that. It now says it was merged and closed and that I can delete my branch.

[tetrapod00]

Huh. I guess if you force push a completely empty PR it considers it the same as merging that empty PR? That makes a certain kind of sense.

At this point I would just remake it as a new PR...

[raziel444]

That does seem to be the case (this Stack Overflow looks awfully similar: https://stackoverflow.com/a/46053849)
I will try again with a new PR tomorrow. Sorry for the inconvenience!