search

Orama-Interactive / Pixelorama

Unleash your creativity with Pixelorama, a powerful and accessible open-source pixel art multitool. Whether you want to create sprites, tiles, animations, or just express yourself in the language of pixel art, this software will realize your pixel-perfect dreams with a vast toolbox of features. Available on Windows, Linux, macOS and the Web!
https://orama-interactive.itch.io/pixelorama
MIT License
6.46k stars 351 forks source link

Make default palettes available for clean installs on MacOS #1008

Closed haythamnikolaidis closed 2 months ago

haythamnikolaidis commented 2 months ago

This PR aims to resolve : 791

On MacOS with a clean install the application is unable to reference the pixelorama_data directory in the Pixelorama.app/Contents/Resources directory, this meant that Palettes.gd was unable to copy the default palette files to the user directory.

The above was cause by 3 issues:

  1. root_directory in Globals.dg's _init() function was set to . which resolved to the directory of the binary (Pixelorama.app/Contents/macOS/)
  2. The is_instance_valid() seemed to always return false in _get_palette_files() in Palettes.gd while running as an exported application (it worked well in the editor.)
  3. Even with the pixelorama_data directory being copied to the Pixelorama.app/Contents/Resources directory, DirAccess would fail to open the Resources directory.

The solution I have implemented is to set the root_directory to "res://" for MacOS, so that it maps to the virtual directory of the Pixelorama.pck file. Check the validity of dir in _get_palette_files() with a null check that should cover both of the previous conditions, eliminating the need for is_instance_valid(). Then (probably controversially) removed the .gdignore file from the pixelorama_data directory before export of the release builds, this ensure that the pixelorama_data directory is packed into the Pixelorama.pck file and available at run time.

I was unsure why the .gdignore file was there to begin with, but I was hesitant to remove it all together incase it interfered with the other platforms, therefore I thought removing it in the automated builds made the most sense, to keep this as a change that purely affects MacOS.

haythamnikolaidis commented 2 months ago

I reverted the changes to the workflows as well as the root_directory variable. It now resolves finding the pixelorama_data directory by adding the Pixelorama.app/Content/Resources directory to the data_directories list.

This resolves the original problem, without affecting override.cfg or needing to change the automated builds.

This leaves me with a new concerns though:

On start up the app will copy the default palettes to the /Library/Application Support/pixelorama/ (user://) directory. The code always checks to see if the default palettes are in the user:// directory, if they are not it copies them over again. This means renaming the default palette to anything else will recreate it as default, leaving the user with two default palettes. I replicated this on both MacOS and windows, however it seems to only affect the main branch as it does not happen in 0.77.4 Should we create a new issue for this?

OverloadedOrama commented 2 months ago

I reverted the changes to the workflows as well as the root_directory variable. It now resolves finding the pixelorama_data directory by adding the Pixelorama.app/Content/Resources directory to the data_directories list.

Awesome, thank you!

On start up the app will copy the default palettes to the /Library/Application Support/pixelorama/ (user://) directory. The code always checks to see if the default palettes are in the user:// directory, if they are not it copies them over again. This means renaming the default palette to anything else will recreate it as default, leaving the user with two default palettes. I replicated this on both MacOS and windows, however it seems to only affect the main branch as it does not happen in 0.77.4 Should we create a new issue for this?

Yeah, I think a new issue should be created for this. Since it doesn't happen on macOS only, it's a general issue and it should be unrelated to this PR and #791. If this PR is ready to get merged, I think I should go ahead and merge it now and we can fix that other issue later.

haythamnikolaidis commented 2 months ago

I am happy that we merge this. I am also happy to create the issue to track the new default palettes bug.