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

[iOS] Sync the boot splash and the launch screen image scale modes #102668

Merged

Conversation

jkirsteins
Copy link
Contributor

What does this PR address?

Using iOS launch screens for the boot splash is not aligned with how Godot renders the boot splash.

On iOS the splash screen with launch images happens in two stages:

  • show the launch screen (the export template allows modifying the scale mode - aspect fit, aspect fill etc.)
  • afterwards Godot takes over, and renders the splash screen. Godot does not take into account the iOS splash scale mode so when it takes over, there is a visual tear as the splash changes dimensions and position.

This is partially addressing #81389 (though this PR does not address any compatibility issues with the legacy renderer)

Change outline

  • I added two methods in Vector2 for fitting Vector2 inside another Vector2, either with aspect-fit-center or aspect-fit-cover modes (analogous to what the TextureRect exposes)
  • I added Vector2 tests to verify the calculated areas
  • I refactored RendererCompositorRD to allow the OS singleton to calculate the splash area. The iOS singleton will fetch the aspect mode from Info.plist and calculate a matching area. Others will keep existing behavior.

Testing

Here's some visual examples of the effect.

Landscape - export template specifies "scale to fill"

Before the changes:

bad__landscape__keepAspectCovered__scaleToFill.mov

After the changes:

good__landscape__keepAspectCovered__scaleToFill.mov

Portrait - export template specifies "scale"

Before the changes:

bad__portrait__scale__scale.mov

After the changes:

good_portrait__scale__scale.mp4

@jkirsteins jkirsteins requested review from a team as code owners February 10, 2025 18:12
@Calinou Calinou added bug platform:ios topic:porting cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Feb 10, 2025
@Calinou Calinou added this to the 4.4 milestone Feb 10, 2025
@bruvzg bruvzg self-requested a review February 10, 2025 18:14
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Seems to be working as expected.

@jkirsteins jkirsteins force-pushed the janiskirsteins/ios-aspect-fill-splash branch 5 times, most recently from 79976cb to 7ba2049 Compare February 11, 2025 11:05
@AThousandShips
Copy link
Member

Please try to avoid pushing so many times in a row (10 times in 35 minutes), it really puts pressure on the automatic checks

@jkirsteins
Copy link
Contributor Author

Please try to avoid pushing so many times in a row (10 times in 35 minutes), it really puts pressure on the automatic checks

Apologies, I didn't realize. Will avoid that.

@akien-mga
Copy link
Member

akien-mga commented Feb 11, 2025

The additions to Vector2 and OS seem highly specific and not really warranted to be core additions.
Edit: For OS I see it's needed for rendering so I guess it's fine.

Please add such code as helper methods where it's needed and not in core - see https://docs.godotengine.org/en/stable/contributing/development/best_practices_for_engine_contributors.html#prefer-local-solutions

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I don't see (a priori) a reason to add a new API to Vector2 to calculate some boot splash metrics.

In second review, I see the OS one is used in RendererRD so I guess that's needed, though that still feels a bit weird architecture wise.

@jkirsteins
Copy link
Contributor Author

Thanks for the prompt feedback everyone.

I don't see (a priori) a reason to add a new API to Vector2 to calculate some boot splash metrics.

I'll move the methods. But just to share my thought process - I chose adding them to the core, because the functionality itself (fitting a size inside another size given a stretch mode) is not specific to iOS. In fact, this code already exists (it is essentially what TextureRect does with its stretch modes). But since that code is located in TextureRect, I couldn't cleanly reuse it.

So my rationale was that by putting this in core, it could help unify this type of aspect fitting logic in the future.

However, I see how my approach does not fully align with existing conventions, so I'm happy to move it.

In second review, I see the OS one is used in RendererRD so I guess that's needed, though that still feels a bit weird architecture wise.

An obvious other solution could be to have an OS-specific branch in the RendererRD, but I think it is also not ideal (there is no precedent to branch like that in this class). However, as it's a small change specific to just one platform, maybe that's preferable, instead of complicating the OS API.

Please let me know if you prefer me to change the singleton approach as well.

Next steps:

  • I'll move the new Vector2 methods to the iOS singleton later today, and
  • I'll keep the OS singleton approach for now, unless there's an explicit ask to change that.

@akien-mga
Copy link
Member

Next steps:

  • I'll move the new Vector2 methods to the iOS singleton later today, and
  • I'll keep the OS singleton approach for now, unless there's an explicit ask to change that.

Sounds good to me!

I'll move the methods. But just to share my thought process - I chose adding them to the core, because the functionality itself (fitting a size inside another size given a stretch mode) is not specific to iOS. In fact, this code already exists (it is essentially what TextureRect does with its stretch modes). But since that code is located in TextureRect, I couldn't cleanly reuse it.

That's a good point. Our current guidelines would likely be to hardcode it again in iOS for now, but look later if we want to refactor and harmonize this code with a core API. We often do that to first confirm that there's a frequent need for a bit of API before solidifying it in the core.

@jkirsteins jkirsteins force-pushed the janiskirsteins/ios-aspect-fill-splash branch from 7ba2049 to 0e266b0 Compare February 12, 2025 16:42
@jkirsteins
Copy link
Contributor Author

Pushed an update that moves the 2 methods to iOS. I also removed the tests I had added to Vector2 (they were useful to validate everything while it was WIP, but not sure they would add a lot of value in the iOS singleton).

I already squashed it, but in retrospect I realize that makes it harder to see what I changed. I'll wait for the review to be finalized next time before squashing, sorry about the inconvenience.

Copy link
Member

@akien-mga akien-mga 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 to me!

@Repiteo Repiteo merged commit 78f1918 into godotengine:master Feb 12, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 12, 2025

Thanks!

@jkirsteins jkirsteins deleted the janiskirsteins/ios-aspect-fill-splash branch February 13, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release platform:ios topic:porting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants