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

Complete GUI Refactor #25

Merged
merged 6 commits into from Apr 6, 2022
Merged

Conversation

blessedcoolant
Copy link
Contributor

This patch brings in a massive number of changes to the frontend of the application. Please feel free to discuss the proposed changes with me at any time.

Implemented Recoil as a state management system.

Why Recoil? It is a robust library built by developers at Facebook for state management. It has an extremely simple API for implementation that is in sync with React syntax compared to any other state management system out there and works amazingly well. While the official release status is beta as it becomes fully featured, the library is already used in various systems at Facebook and is very stable for the use cases of this application.

Why global state management? One of the major issues I saw with the current file structure is that there is minimal code splitting and it makes further development of the frontend a cumbersome task. I have broken down the frontend into various easy to access components isolating the GUI from the logic. To avoid prop drilling, we need global state management to handle the necessary tasks. This will also facilitate the addition of any new features greatly.

Code Splitting. Majority of the components that can be isolated in the application have now been done so.

All New Custom CSS & Removal of Tailwind

While Tailwind is a great way to deploy beautiful interfaces quickly, anyone trying to stylize the application further needs to be familiar with Tailwind which makes it harder for more people to work on it. Not to mention, I am not a particular fan of flooding JSX elements with inline CSS classes. That makes reading the code extremely hard and bloats up component code drastically.

As a replacement to Tailwind, I implemented a custom styling system using SCSS as a developer dependency.

In the new system, all the general and shared styles are in the styles folder and all the component styles are in the same folder as the component for easy access. The _index.scss file now acts as a central import for every other stylesheet that needs to be loaded.

What Changed?

  • The entire application looks and feels like the current implementation with minimal changes.
  • The green (#bdff01) highlight used in the application has now been changed to a bright yellow (rgb(255, 190, 0)) because I felt it better suited the new Dark Mode (see below).
  • The swipe bar for comparing before and after images has now been removed and instead the comparison is a smooth fade effect. I felt this was better to analyze image changes rather than a swiper. // Can add the swipe back if needed.

Dark Mode

A brand new Dark Mode has been added for the application. Users can enable and disable by tapping the button in the header or by using the Shift + D hotkey.

Other New Features

  • When the editor image is now zoomed out to its default size, the image now also gets centered back.
  • Various minor bug and QoL fixes

TODO - Stuff I might work on next

  • The currently used react-zoom-pinch-pan module is not mobile friendly. It does not allow brush strokes. Need to figure out a way to fix this.
  • Further optimization of the frontend code with better code splitting and performance.
  • When using the LaMa model, the first stroke has a delayed response from the backend but the ones that follow are almost immediate. I believe this is happening because of the initialization of the model on the first stroke. I wonder if either of us can look at it and see if this can somehow be preloaded so the user experience is smooth from the first stroke.
  • Enable threading for the desktop application mode so flaskwebgui does not block the main applications Python console.

blessedcoolant and others added 3 commits March 26, 2022 04:10
This patch brings in a massive number of changes to the frontend of the application. Please feel free to discuss the proposed changes with me at any time.

Implemented Recoil as a state management system.
Why Recoil? It is a robust library built by developers at Facebook for state management. It has an  extremely simple API for implementation that is in sync with React syntax compared to any other state management system out there and works amazingly well. While the official release status is beta as it becomes fully featured, the library is already used in various systems at Facebook and is very stable for the use cases of this application.

Why global state management? One of the major issues I saw with the current file structure is that there is minimal code splitting and it makes further development of the frontend a cumbersome task. I have broken down the frontend into various easy to access components isolating the GUI from the logic. To avoid prop drilling, we need global state management to handle the necessary tasks. This will also facilitate the addition of any new features greatly.

Code Splitting. Majority of the components that can be isolated in the application have now been done so.
All New Custom CSS & Removal of Tailwind
While Tailwind is a great way to deploy beautiful interfaces quickly, anyone trying to stylize the application further needs to be familiar with Tailwind which makes it harder for more people to work on it. Not to mention, I am not a particular fan of flooding JSX elements with inline CSS classes. That makes reading the code extremely hard and bloats up component code drastically.

As a replacement to Tailwind, I implemented a custom styling system using SCSS as a developer dependency.

In the new system, all the general and shared styles are in the styles folder and all the component styles are in the same folder as the component for easy access.The _index.scss file now acts as a central import for every other stylesheet that needs to be loaded.

What Changed?
The entire application looks and feels like the current implementation with minimal changes.
The green (#bdff01) highlight used in the application has now been changed to a bright yellow (rgb(255, 190, 0)) because I felt it better suited the new Dark Mode (see below).
The swipe bar for comparing before and after images has now been removed and instead the comparison is a smooth fade effect. I felt this was better to analyze image changes rather than a swiper. // Can add the swipe back if needed.

Dark Mode
A brand new Dark Mode has been added for the application. Users can enable and disable by tapping the button in the header or by using the Shift + D hotkey.

Other Misc New Features
When the editor image is now zoomed out to its default size, the image now also gets centered back.

TODO
The currently used react-zoom-pinch-pan module is not mobile friendly. It does not allow brush strokes. Need to figure out a way to fix this.
Further optimization of the frontend code with better code splitting and performance.
When using the LaMa model, the first stroke has a delayed response from the backend but the ones that follow are almost immediate. I believe this is happening because of the initialization of the model on the first stroke. I wonder if either of us can look at it and see if this can somehow be preloaded so the user experience is smooth from the first stroke.
Enable threading for the desktop application mode so flaskwebgui does not block the main applications Python console.
@Sanster
Copy link
Owner

Sanster commented Mar 28, 2022

Recoil for state management

I have experience using redux, and I have always wanted to learn about other state management libraries. Recoil is one of them. Global state management is very important for subsequent development. I am very supportive of introducing such a library.

Can you help add a simple test example, so that developers like me who are not familiar with the front-end can refer to your example to write?

Custom styling

I don't have a particular preference for tailwind or scss, but I think you're right, separate style files are indeed more conducive to development collaboration. But I have some suggestions for the revised style.

  1. The brush is too inconspicuous and a little hard to find

截屏2022-03-28 下午9 03 13

  1. The color of the interface is a bit too much, it feels a bit messy. One of my favorite things about the cleanup.pictures original implementation is that the whole design looks clean and has a clear theme color.

截屏2022-03-28 下午9 06 31

截屏2022-03-28 下午9 05 04

Fade effect

I personally prefer swipe bar design for before and after image comparison, it feels more impactful and more obvious. The fade effect may be more suitable for this feature request? I open an issue to discuss this: #26.

Dark Mode

When I open the page for the first time, and it's dark mode, I'm confused about this switcher icon. Maybe we can change to an icon more like ☀️

截屏2022-03-28 下午9 31 18

@blessedcoolant
Copy link
Contributor Author

blessedcoolant commented Mar 28, 2022

@Sanster Thank you for the feedback.

Recoil for state management.

I'd be more than happy to write test examples. The Recoil docs are very clear but having some local app related examples might be better. Would you like me to include them as a file or as a part of documentation?

Styling Issues

I'll fix up those issues. I will either tweak the brush color or the UI accent to match up so that's one less color used.

As for the brush slider, I actually did not theme it at all. I have been trying to see what the best way to style it would be because the input[range] element display extremely differently across different browsers / devices. So I was finding the best way to overwrite this with minimal CSS. I'll fix this up too.

Fade Effect

I'll reimplement the swipe for now. As for that feature request, would we want to keep both kind of transitions for this? Because his request is basically the same as a before and after compare but with a masked highlight on top of that. I was thinking the best way to implement this would be to add a check button. If a user checks that button, it'll display the highlight when you swipe, else it won't.

Dark Mode

Yeah. I just threw on a quick circle in CSS because I wanted to use as few images as possible to keep the initial load fast. But I think I can make a sun shape in CSS too. If not, I'll bring in an SVG.

I'll push some updates with some of these changes in a while.

@Sanster
Copy link
Owner

Sanster commented Mar 29, 2022

  • Recoil for state management: It is better to add a runnable test file
  • Fade Effect Masked highlight: Check button idea sounds good

@blessedcoolant
Copy link
Contributor Author

Deployed Patch 1

With all the changes you requested.

Brush & Stroke Color and Brush Slider

Adjusted the brush shape color and stroke color to match the new accent color eliminating the previous green entirely. This shade of yellow is clearly visible on white and dark backgrounds. The brush slider has now been themed fully. Hopefully it displays correctly across all browsers (tested Chrome, Firefox, Edge). It loads as default on mobile (Safari) but I’ll fix that once I do a mobile only patch to fix all mobile issues. Seems to be working fine or else.

Reworked Size Selector

Reworked the working of the Size Selector. It was initially drawing the default size from localStorage or defaulting to1080px. This might have been okay for a single image load, but applying the same settings for any subsequent image loading meant that the initial canvas size and the root inpaint setting did not match with the max resolution of the new input image. Instead of this, I got rid of the option from localStorage and now let the inputImage determine the sizeLimit. Now the inpainter will take this value automatically from the image loaded instead. This also avoids loading of a 1080px default inpainter replies if the user does not actually trigger the full size. It was finicky.

I also overhauled the GUI for it. No longer using the select tag (which would have been more convenient) – but some internal code in the ‘react-use’ module was causing unnecessary re-renders and triggers. I could replace react-use entirely with custom hooks but that’s for another day. For now, it seems to be working fine. I’ve done some extensive testing on this to fix up any bugs I could find but if you find any, please let me know.

Comparison Slider

Added my own version of the slider bar. It functions more or less the same as what it was before. I will style the slider bar itself in a future patch to make it more theme and image friendly.

Recoil Example

Added a RecoilExample.tsx file with some basic functions and some comments on how to use Recoil in the app for developers. I covered most of the basic aspects that are in the scope of this application. If there’s a need, I can expand on other Recoil features later.

Mobile

I made the mobile UI too but sadly at the moment the app is unusable on a mobile interface because the react-pinch-zoom module that you use to zoom and pan the canvas isn’t set up to work with touch. I’ll try to figure out how to do that with this library (the documentation is pretty bad) .. or replace it with a new library or write my own code.

Dark Mode Icon

Swapped out the current icon designs for something even more obvious from heroicons.

Misc Changes

Improved the toolkit background at the bottom so the contents are clearly visible even against dark and light images.
Fixed the canvas not adjusting to window size changes.

Questions

I have some questions that I cannot figure out the answers to. I was wondering if you might have an idea.

So basically the inpainter is fed the file along with the mask in each instance and the result is being displayed. The next inpaint is being fed the file again with a new mask. So what I tried to do was, instead of feeding it the original file, I fed it the new inpainted image. Works as intended but as I keep using the application further, I notice that areas that were previously inpainted now start getting bad artifacts with each inpaint getting worse and worse. This does not happen with the original file as input. Any idea why this happens?

Secondly, I notice that areas that are not a part of the mask get inpainted. Especially some pieces that seem like they’re from older strokes. But the canvas is definitely cleared. I was wondering if that’s just how LaMa handles it or is there some not so obvious bug with the mask being supplied?

@Sanster
Copy link
Owner

Sanster commented Mar 30, 2022

Inpainted state

The inpainted state and eye icon are duplicated, I suggest changing to something like this

Current Suggested
截屏2022-03-30 上午11 41 58 截屏2022-03-30 上午11 42 23

Recoil Example

Sorry for not expressing clearly, actually I want you to help add simple test case example for yarn test. Anyway, there is no test now, we can add it later. So I think there is no need to add RecoilExample.tsx, Recoil doc is the right place to learn about Recoil.

About Questions

Q1: Why the inpainting result getting worse and worse when using last inpainted image?

Reasons I can think of: the training of the LaMa model is not a gradual process (and I think it's not necessary because the original image can definitely retain more global information than the inpainting result), the mask during training contains multiple strokes, the network is always trained on the original image, not the result of inpainted image. So if we use the result of inpainting as input, the forward behavior is inconsistent with training.

Why do you use the result of inpainting instead of the original image?

Q2: Why areas that are not a part of the mask get inpainted?

As mentioned above, every inpainting forward will use all strokes to generate mask(see example here: #15 (comment)), so older stroke regions may produce different results than before becauce the input of the network is not exactly the same(although the mask of older strokes does not change).

The way to maintain consistency of older stroke regions is to run the model once per region(This is the current behavior of --crop-trigger-size.), but this will reduce the efficiency of model inference.

@blessedcoolant
Copy link
Contributor Author

@Sanster I fixed the Inpainted State. Only the eye icon now. I also removed the Recoil example. I will make a proper test file for it later.

Why do you use the result of inpainting instead of the original image?
Because my idea was that the inpainted image would have more new pixel data that might help other difficult areas further down the inpainting process. Was trying to replicate how Photoshop handles Content-Aware.

@blessedcoolant
Copy link
Contributor Author

@Sanster Anything else needs to be done for this?

@Sanster
Copy link
Owner

Sanster commented Apr 3, 2022

@blessedcoolant Thanks for this PR, I'm still tweaking the details of the UI to make it look more polished, I'll merge this request when I'm done

@blessedcoolant
Copy link
Contributor Author

@blessedcoolant Thanks for this PR, I'm still tweaking the details of the UI to make it look more polished, I'll merge this request when I'm done

Perfect. Thanks for the update.

@Sanster Sanster merged commit 855fd1f into Sanster:main Apr 6, 2022
@blessedcoolant blessedcoolant deleted the recoil-refactor branch April 8, 2022 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants