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

[Spike] Investigate removing blur from android in order to enable the new architecture #20066

Closed
cammellos opened this issue May 16, 2024 · 16 comments
Assignees
Labels
feature feature requests mobile-core
Milestone

Comments

@cammellos
Copy link
Member

As discussed with designer, is fine to remove blur from android.
Blur on android leads to some performance issue, complexity in the code and prevents us to enable to the new architecture for react native.

In this spike we should quantify the effort needed to remove blur, and what to do instead. @ulisesmac had some opinions on the matter.

After this spike, we should be able to answer roughly the questions:

  1. how much effort is to remove blur
  2. what are we going to do instead
  3. is this enough to enable the new architecture on android or there are more blockers
@cammellos cammellos added feature feature requests mobile-core labels May 16, 2024
@cammellos cammellos added this to the 2.30.0 Beta milestone May 16, 2024
@siddarthkay
Copy link
Contributor

siddarthkay commented May 16, 2024

is this enough to enable the new architecture on android

most likely just this.
ref -> #18138 (comment)
We do not have build time failures, we just have this ugly pink thing which indicates incompatibility

@ilmotta
Copy link
Contributor

ilmotta commented May 16, 2024

I will grab this issue probably early next week due to other priorities. Until then, if someone wants to do it no problem 👍🏼

@ulisesmac ulisesmac self-assigned this May 16, 2024
@ulisesmac
Copy link
Contributor

I will grab this issue probably early next week due to other priorities. Until then, if someone wants to do it no problem 👍🏼

Please whoever from mobile that's going to work on this too, assign it to yourself along with me :)

@ulisesmac
Copy link
Contributor

ulisesmac commented May 21, 2024

An update on the research of this issue:

Approach 1 (not good)

If we just replace the blur-view with a view the app is not usable, since many views are just transparent:

Screen_Recording_20240521_145858_Status.Debug.mp4

Approach 2

Instead we can try finding the color being used and use an overlay with transparency.
Example, for shell we have neutral-80-opa-80 so if we change it for neutral-80 with opacity 92 (magic number I picked), it'd look as follows:

Screen_Recording_20240521_150017_Status.Debug.mp4

We have 25 usages of the blur-view component and 4 of the reanimated's blur, it'd be great if we find an abstraction to make this substitution easier, but even if not, I think it's doable.

Approach 3

Just use a solid color instead of finding a new opacity.

Important notes

I tested this on a development build running on a Samsung Galaxy S23 Ultra, I felt the app is still slow 🤔 and I thought the improvement would be more significant. According to this issue the problems we have are:

  1. Performance issue,
  2. Complexity in the code
  3. Blur prevents us to enable to the new architecture for react native.

Regarding to 1:

For sure the performance is improved, but IMO it's barely noticeable, maybe in some screens or other devices the improvement is more clear. It'd be great if we properly compare the performance before applying a solution.

And answering a question from this issue:

How much effort is to remove blur

Not too much in terms of refactoring, I'd say we need some days to properly refactor the code
In the case we can't a color that looks good, we'll need to align with designers, so probably it'll delay it a bit more.

Code

This is the code I used to test it (just a quick attempt) in react-native.blur

(def view* (reagent/adapt-react-class (.-BlurView blur)))

(defn view- []
  (let [this     (r/current-component)
        props    (r/props this)
        children (r/children this)]
    (if platform/android?
      (into [rn/view props
             [rn/view {:style {:position         :absolute
                               :top              0
                               :left             0
                               :right            0
                               :bottom           0
                               :background-color (colors/alpha colors/neutral-80 0.92)
                               :z-index          -10}}]]
            children)
      (into [view* props] children))))

(def vv (r/reactify-component view-))

(def view (r/adapt-react-class vv))

@ulisesmac
Copy link
Contributor

is this enough to enable the new architecture on android or there are more blockers

Sid is the expert on it, @siddarthkay could you please share with us the limitations you've found to enable it?

@ilmotta
Copy link
Contributor

ilmotta commented May 22, 2024

Thank you @ulisesmac. Excellent review of the solutions 👏🏼

To me, the main reason besides the ones you wrote is the cost of development for very little return to users, particularly for a product that still doesn't work well in so many areas. Even the fact that we're still discussing about blur in so many meetings and issues shows just how much wasted effort this is.

I surely like the blur implementation when we can nail it, but on Android I would vote for a simple solid replacement, the simplest and cheapest for now. I understand the concepts behind blur, but I believe users will be capable of understanding the context they are in and how things overlap without blur and without transparency (on Android). Removing blur probably won't affect revenue growth.

Unfortunately, as you correctly pointed out, we should now account for the cost of removing blur, which obviously not just pressing a magic button. I think if it's 2-3 days the cost of removal will pay off in the medium to long run. Just imagine that one more issue from a developer using blur can easily force that developer to waste hours or more and this cost is not easily measurable, but it's there.

@siddarthkay
Copy link
Contributor

the limitations you've found to enable it?

At the moment we can't get past the login so we will know once blur lib is removed from the project.
All we know so far is that we do not have build issues which is nice, there may or may not be other runtime issues but we can't know for sure till we get past the login screen :)

@ulisesmac
Copy link
Contributor

@ilmotta

but on Android I would vote for a simple solid replacement, the simplest and cheapest for now.

Yeah, it's the easiest, we can take this approach.


@siddarthkay

At the moment we can't get past the login so we will know once blur lib is removed from the project.

"from the project" means that we also need to remove it from iOS? 😮

@flexsurfer
Copy link
Member

"from the project" means that we also need to remove it from iOS?

i think it will be removed only for Android, conditionally I guess

@flexsurfer
Copy link
Member

thank you @ulisesmac i would vote for the simple solid replacement, and It might not that visible improvement but we don't have like one big thing we can improve, overall app performance is the sum of small things like blur, so we have to improve them all

@siddarthkay
Copy link
Contributor

"from the project" means that we also need to remove it from iOS?

I assumed that would be easier for whoever is implementing the fallback.

But as long as BlurView component is not rendered on Android thats all we need to move forward in enabling new architecture for react-native.

This can be achieved either by nuking the library OR by adding platform checks in the component we use for Blur View, I'd vote for whatever is easier/faster :)

@flexsurfer
Copy link
Member

This can be achieved either by nuking the library OR by adding platform checks in the component we use for Blur View, I'd vote for whatever is easier/faster :)

unfortunately, there are no options, we should keep it for iOS

@ilmotta
Copy link
Contributor

ilmotta commented May 23, 2024

@cammellos, since you opened this issue, do you think we have a satisfactory result and can close it? The consensus seems to be on using a solid color replacement. When do you think we should execute this decision to replace blur views? (the question goes to everyone else as well)

One of the last missing pieces is double-checking with designers the solid colors they would like to see been used on Android and if they can attach them in Figma to avoid the case of devs picking up colors by personal preference, but that can be done in a separate issue when we actually replace blur views. So not a blocker for us.

@ulisesmac
Copy link
Contributor

Ok, one last thing I want to add.

We can use React Native 0.74 (latest) with the blur library (https://github.com/Kureev/react-native-blur) and it compiles and works on Android, I'd say the same as before.

I confirmed it by updating a personal app that uses blur and that now it's running on RN 0.74, of course Status is a lot more complex, but I could use them without problems.

I know we have other arguments to remove the blur, and I'm not against it, I'm just sharing that maybe the blur is not a blocker for upgrading.

@siddarthkay
Copy link
Contributor

blur is not a blocker for upgrading.

Yeah we're not blocked for upgrading react-native, we're blocked for enabling new architecture in react-native.

We also have this in our react-native.config.js file ->

platforms: {
android: null,
},

Which is probably why I saw this when trying to enable new architecture -> #18138 (comment)

With regards to upgrading react-native to 0.74 we discussed it in the offsite that we would rather not do it before July release because RN upgrades may cause some unknown side effects.

It is worth noting that last upgrade was smooth and didn't create many side effects, but some folks still feel the after effects of the ones before that :D

@cammellos
Copy link
Member Author

@ilmotta yes, thank you, I'll close this one and create an issue that is actionable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feature requests mobile-core
Projects
Status: Done
Development

No branches or pull requests

5 participants