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

Update UI in nodes-and-scenes #10589

Merged
merged 1 commit into from
Feb 2, 2025

Conversation

gianfun
Copy link

@gianfun gianfun commented Jan 31, 2025

This PR updates screenshots from the nodes-and-scenes step from the step-by-step guide.

There are two main changes:

  • The Create New Node window now specifies the Class much more clearly.
  • The Label node has a few extra properties

Questions:

  • Are image dimensions important? Should I take care to make them the exact size they were before? (Did not see anything in the guide about this)

Image diffs:

File: getting_started/step_by_step/img/nodes_and_scenes_01_empty_editor.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_01_empty_editor.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_01_empty_editor.webp

File: getting_started/step_by_step/img/nodes_and_scenes_03_create_node_window.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_03_create_node_window.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_03_create_node_window.webp

File: getting_started/step_by_step/img/nodes_and_scenes_04_create_label_window.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_04_create_label_window.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_04_create_label_window.webp

File: getting_started/step_by_step/img/nodes_and_scenes_05_editor_with_label.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_05_editor_with_label.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_05_editor_with_label.webp

File: getting_started/step_by_step/img/nodes_and_scenes_08_hello_world_text.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_08_hello_world_text.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_08_hello_world_text.webp

File: getting_started/step_by_step/img/nodes_and_scenes_10_save_scene_as.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_10_save_scene_as.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_10_save_scene_as.webp

File: getting_started/step_by_step/img/nodes_and_scenes_11_final_result.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_11_final_result.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_11_final_result.webp

File: getting_started/step_by_step/img/nodes_and_scenes_14_select_main_scene.webp

Old:
Old getting_started/step_by_step/img/nodes_and_scenes_14_select_main_scene.webp

New:
New getting_started/step_by_step/img/nodes_and_scenes_14_select_main_scene.webp

@tetrapod00 tetrapod00 added enhancement area:getting started Issues and PRs related to the Getting Started section of the documentation content:images Issues and PRs involving outdated or incorrect images in articles cherrypick:4.3 labels Jan 31, 2025
tetrapod00
tetrapod00 previously approved these changes Jan 31, 2025
Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Looks good!

In the future you don't need to include the images in the PR description. But if you want to take the time, it can help, because GitHub's tools for comparing webp files are really bad

As for width:

  • As a rule, new images don't have to match old image dimensions exactly.
  • Image width must be less than 1920 pixels (we have a rule that images must be smaller than 1080p, which is 1920x1080)
  • UI screenshots should be less than ~796 pixels wide if you can, so that they fit in the content column of the docs with no scaling issues. But this isn't a hard rule and it's not written down anywhere. You can't always make screenshots of the whole editor this small, either.

So overall your images fit the guidelines just fine.

@gianfun
Copy link
Author

gianfun commented Jan 31, 2025

Cool, thanks!
Yeah, once I realized github would not show the diffs I ended up adding them to the PR description ):

Given the images currently fir the guidelines (and that image 3 and 4 have the same width), I'll leave them as is, then. Thanks!

@skyace65
Copy link
Contributor

skyace65 commented Jan 31, 2025

I'm going to say no to merging this PR. We're a month or less out from 4.4 and will need to update these screenshots for that anyway because of the new game workspace. In addition you did Lossy compression on these images which we explicitly do not want, and I can see the artifacts from compression.

@gianfun
Copy link
Author

gianfun commented Feb 1, 2025

Sure, no problem!
I can screenshot and not apply any compression, if it makes sense (so that we have the docs updated until 4.4 launches), but I'm ok with just closing this PR, too!
Or, alternatively, does it make sense building from master and having those images, instead, in order to decrease work for then 4.4 releases?
(If not, feel free to close this PR!)

@tetrapod00
Copy link
Contributor

I think retaking the screenshots, and only targetting 4.4 would be fine.

  • No need to build from master, you can use one of the official beta builds: https://godotengine.org/article/dev-snapshot-godot-4-4-beta-2/. I believe since we're in beta we shouldn't be getting any more changes that break screenshots
  • Make sure that the original screenshots you're taking are lossless, so either lossless PNG or webp, depending on how you're capturing them originally
  • You can compress (and convert from png if necessary) with squoosh. These are the settings I use but I'm not an expert on best practices here. I usually capture in PNG, convert in krita, then squoosh if necessary
    firefox_d0DE7EHMhV

@skyace65 Does that sound okay, did I miss any steps here?

@tetrapod00 tetrapod00 dismissed their stale review February 1, 2025 02:08

Should target 4.4 instead

@skyace65
Copy link
Contributor

skyace65 commented Feb 1, 2025

Tetrapod's instructions are perfect, and those are the squoosh settings I use for my own doc images. So yes if you want you can re-do this PR with screenshots from 4.4. You can either build from master like you said, or use the latest beta from the Godot website, either is fine.

@gianfun gianfun force-pushed the update-nodes-and-scenes-ui branch from ebd4294 to 96d6cdd Compare February 1, 2025 16:27
@gianfun
Copy link
Author

gianfun commented Feb 1, 2025

Updated images using 4.4, but just realized we do not have a 4.4 branch yet. Do I leave it based on master for now and we wait until 4.4 is released?
(I'm going to have lunch but when I come back I'll post the image diffs on the PR description)

@skyace65
Copy link
Contributor

skyace65 commented Feb 1, 2025

For upcoming releases you just use the master branch. Generally you should always use the master branch except for specific circumstances.

As an example, say 4.4 releases in a month and two weeks later you want to make a PR that applies to 4.4 and the master branch. You would make the PR for the master branch, and we would add a "cherrypick 4.4" label to the PR, so the contents of that PR would be added to the 4.4 branch when we do the next round of cherrypicks.

You would only make a PR for the 4.4 branch, 4.3, etc, if the change you're making applies to that branch but does not apply to the master branch, or if it's a change that was already made to the master branch that can't be cherry picked easily

Copy link
Contributor

@tetrapod00 tetrapod00 left a comment

Choose a reason for hiding this comment

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

Overall looks okay to me. This time I checked for lossy artifacts.

@gianfun gianfun force-pushed the update-nodes-and-scenes-ui branch from 96d6cdd to b38b1bc Compare February 1, 2025 23:03
@gianfun
Copy link
Author

gianfun commented Feb 1, 2025

For upcoming releases you just use the master branch. Generally you should always use the master branch except for specific circumstances.

As an example, say 4.4 releases in a month and two weeks later you want to make a PR that applies to 4.4 and the master branch. You would make the PR for the master branch, and we would add a "cherrypick 4.4" label to the PR, so the contents of that PR would be added to the 4.4 branch when we do the next round of cherrypicks.

You would only make a PR for the 4.4 branch, 4.3, etc, if the change you're making applies to that branch but does not apply to the master branch, or if it's a change that was already made to the master branch that can't be cherry picked easily

Oh, I was under the impression that merging to master would publish to the website instantly. Had forgotten that it needs to then be cherry picked to update the 4.3 docs.

Thanks for the explanation!

@skyace65 skyace65 added this to the 4.4 milestone Feb 2, 2025
@skyace65 skyace65 merged commit d4a7065 into godotengine:master Feb 2, 2025
1 check passed
@skyace65
Copy link
Contributor

skyace65 commented Feb 2, 2025

Thanks! And congrats on your first merged PR!

@gianfun gianfun deleted the update-nodes-and-scenes-ui branch February 2, 2025 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:getting started Issues and PRs related to the Getting Started section of the documentation content:images Issues and PRs involving outdated or incorrect images in articles enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants