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

Replace old palette system with a new one #447

Merged
merged 10 commits into from
Apr 16, 2021

Conversation

novhack
Copy link
Contributor

@novhack novhack commented Jan 30, 2021

I completely remade the palette system. Basically all features from #590 are implemented except undo/redo support.

This pull request breaks compatibility with old palettes. When I was redesigning the load/save system I decided to go with Resource format for palettes so the old JSON won't load anymore. Keeping legacy JSON related code seemed like a bad idea.

image
New dialogs:
image
image

@kleonc
Copy link
Contributor

kleonc commented Jan 31, 2021

Issues (not sure if related to undo/redo being not implemented yet):

GUI is not being instantly updated when modyfying palettes loaded from the disk (not created at the current Pixelorama run).

Godot_v3 2 3-stable_win64_SrjKZRXRIA
Bk05LKWNOK

Editing palette loaded from the disk (not created at the current Pixelorama run) duplicates the entry in the palettes dropdown.

kVpm5SAOHU

@novhack
Copy link
Contributor Author

novhack commented Jan 31, 2021

Thank you for testing. What operating system do you use? I see that the path for the palette is in res:// which doesn't happen to me - all imported palettes should be stored/copied to appdata.

@kleonc
Copy link
Contributor

kleonc commented Jan 31, 2021

Windows 7. I was running this version in debug mode, right from the Godot.

All I have in appdata are: a backup, config file and some logs.
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\backup-1609273475-0
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\cache.ini
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\logs
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\logs\godot.log
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\logs\godot_2021-01-31_13-04-56.log
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\logs\godot_2021-01-31_13-05-38.log
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\logs\godot_2021-01-31_13-09-35.log
C:\Users\MyUsername\AppData\Roaming\Godot\app_userdata\Pixelorama\logs\godot_2021-01-31_14-20-33.log

I've just downloaded current official version (Pixelorama [Windows 64-bit].zip) from the site and it has pixelorama_data directory right next to the .exe and that's where palettes are being added/edited. Same happens in your version: it's indeed modyfying contents of ./pixelorama_data/Palettes/.

Copy link
Member

@Erevoid Erevoid left a comment

Choose a reason for hiding this comment

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

Eyo!

I tested your PR, I really like the new palette system in general. I'd like to comment some details I figured out while playing with the settings:

  1. When you put high values on width and height it can crash the program. For example I put 100 for both width and height, and Pixelorama froze for several seconds. When I tried to add a color, it crashed. Μaybe have a limitation as to how many colours can be added with relatively no problem in most devices?
  2. You can have 0 as the value for width or height, if there is no specific reason for that, why not have a minimum value of 1?
  3. When you select a colour from the palette and then you switch the colour from the colour picker (not the palette's colour), the palette colour remains selected. That's a bit confusing since you have a specific colour shown at the colour picker and a different one selected at the palette, so I suggest deselecting the palette colour when the user changes the colour of the colour picker.
  4. When the user creates a new palette it's not clear enough that a name is required so that the Ok button can be pressed. That can be fixed by adding some red text indicating that the name is missing when the field is empty

@novhack
Copy link
Contributor Author

novhack commented Mar 22, 2021

At this point all the issues kleonc and Erevoid had should be resolved.

Palette size now has no impact on performance.
It's possible to scroll with a middle button in the palette panel.
There is now also an importer for "old" json palette format for backwards compatibility.

Hide add/delete color buttons when no palette is displayed.
@novhack
Copy link
Contributor Author

novhack commented Mar 30, 2021

I added two small fixes based on luiq54's feedback on Discord.

@Erevoid
Copy link
Member

Erevoid commented Mar 30, 2021

Didn't find any new issues after the changes, it's looking good in my opinion!

Copy link
Member

@OverloadedOrama OverloadedOrama left a comment

Choose a reason for hiding this comment

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

I think it looks good too. I've been testing it and it seems to be working well, so I'll go ahead and merge it now. Thank you!

@OverloadedOrama OverloadedOrama merged commit 42696e7 into Orama-Interactive:master Apr 16, 2021
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.

4 participants