-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
Merge with main
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.
Recoil for state managementI 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 stylingI 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.
Fade effectI 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 ModeWhen 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 ☀️ |
@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 IssuesI'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 EffectI'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 ModeYeah. 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. |
|
Deployed Patch 1With all the changes you requested. Brush & Stroke Color and Brush SliderAdjusted 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 SelectorReworked 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 SliderAdded 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 ExampleAdded 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. MobileI 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 IconSwapped out the current icon designs for something even more obvious from heroicons. Misc ChangesImproved the toolkit background at the bottom so the contents are clearly visible even against dark and light images. QuestionsI 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? |
Inpainted stateThe inpainted state and eye icon are duplicated, I suggest changing to something like this
Recoil ExampleSorry for not expressing clearly, actually I want you to help add simple test case example for About QuestionsQ1: 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 |
@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? |
@Sanster Anything else needs to be done for this? |
@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. |
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?
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
TODO - Stuff I might work on next