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

Discussion on project files #68

Open
PiMaV opened this issue Nov 5, 2024 · 10 comments
Open

Discussion on project files #68

PiMaV opened this issue Nov 5, 2024 · 10 comments
Labels
discussion Discussion needed first High Prio Needs to be adressed

Comments

@PiMaV
Copy link
Owner

PiMaV commented Nov 5, 2024

As with #67 I would suggest refactoring the "project" loading.
In additione with #63 the whole "project" idea leads more to confusion than benefit.

I would strongly suggest we go back to the roots:

  • LUT can be optionally saved and loaded as json by the user (which is already given)
  • we DONT create .blitz project files for now at all. No seperate handling of options per folder.
  • ALL major settings (no LUT) are stored in a single ASCII config file next to the BLITZ.exe in a config.blitz (as we had with the ini)
  • settings are stored automatically whenever they are changing.
  • Rename "save ini" to "auto-save settings"

REASON:

  • Most of my performance issues are realted to buggy project behaviour
  • Most "normal" users I observed do not understand why the BLITZ is behaving as it is, when "projects" are active ("Why is it doing this now...? Can't I go back...?")
  • We dont have time to debug this properly
  • We have more urgent matters to accomplish for the next release
@PiMaV PiMaV added discussion Discussion needed first High Prio Needs to be adressed labels Nov 5, 2024
@PiMaV PiMaV mentioned this issue Nov 5, 2024
@irkri
Copy link
Collaborator

irkri commented Nov 5, 2024

I don't like the idea of discarding this much work that I put into the project files.
Maybe instead we can just uncheck the "save settings per project" checkbox and let users decide.

For example, applying a mask and cropping data while loading to save memory is a cool feature and good to have. As it also was not easy to implement, I don't see why we should discard it.

I will try and come up with a good solution in the next few days.

If you notice more errors, I definitely need to be able to replicate them in order to fix them properly.

@PiMaV
Copy link
Owner Author

PiMaV commented Nov 5, 2024

I completely understand the investment you've made into the project feature and appreciate its design. My goal, however, is to simplify where possible, reducing user confusion and improving stability—especially with the upcoming release and your departure from the project. I believe a compromise here is essential.

I suggest a bottom-up approach:

  1. Core Program Settings: Settings related to the window, viewer, default, and web functionalities should reside in a single settings.blitz file, as these are machine-dependent (e.g., RAM or multicore thresholds) rather than project-dependent. This plain ASCII file, located next to the executable, would auto-save changes made by the user on the fly.

  2. Project-Dependent Settings: Default values specific to projects, such as size and subset, could move into a new section like project_settings. This approach keeps the settings file compact, containing only essential variables. We could use the existing checkbox for an "auto-save settings" or "Save project-related features" option to cover these aspects. So in a project related .blitz file, we would only keep some the [data] essentials, adding some from the [default] section.

  3. Separate LUT Management: The LUT implementation has shown instabilities. We should fully exclude LUT from .blitz files. The current managing as a standalone save/load JSON operation is good enough for now.

Let me know your thoughts on this structure—particularly on separating project-related settings in a way that maintains user flexibility without reintroducing instability.

@irkri
Copy link
Collaborator

irkri commented Nov 6, 2024

I used the day to implement a new version of settings in BLITZ.

  1. I was wrong about BLITZ needing to restart in order to apply certain window and style changes. It does not do that anymore.
  2. I removed the LUT completely from the settings. Its only possible to load or export it using the corresponding buttons below the LUT.
  3. I split settings into program and project settings, as you suggested. Please have a look if some setting is missing or is put into the wrong category.
  4. Program settings are now saved at all time. With a little more work, I can implement a checkbox for the user to disable that, but right now I just wanted it to work properly.
  5. Project settings are saved and loaded only if the corresponding checkbox is active. The state of the checkbox is saved in the program settings.
  6. In addtition to the previous point, the user can also directly load a .blitz file. This automatically sets up synchronisation with this file no matter the state of the above mentioned checkbox.
  7. At the moment, BLITZ does not allow changing the program settings file. BLITZ does allow changing the project settings file.
  8. Technical detail: Settings are saved by connecting signals of widgets the user interacts with to the QSettings class that manages our program settings and project settings files. These connections need to be cut if a new settings file is loaded. My class _Settings currently is responsible for that.

@PiMaV
Copy link
Owner Author

PiMaV commented Nov 7, 2024

Project Saving Checkbox

  • Suggest changing the "load/save project file" checkbox color to red or making it more dominant when active, emphasizing its importance to prevent users from missing its effect on BLITZ behavior.

Colormap Default Setting

  • Add an option to set a default colormap in settings.blitz. Currently, it defaults to gray+clip. Ideally, this setting should save the last used colormap (e.g., plasma), maintaining consistency across sessions. Note: This isn’t about LUTs, just the default colormap setting.

Subset and Size Ratios

  • subset_ratio and size_ratio should have defaults in settings.blitz but be overridable in project files. This way, the main settings are preserved while allowing project-specific customizations.

BUG: File Drag-and-Drop Limitation

  • When a .blitz file exists, drag-and-drop .blitz files or associated folders dont do anything. It only displays "loading configuration file," but nothing loads (Relative / Absolute path conflict?). File content example:
[General]
path=C:\\DATEN\\KPs\\Measuremement_New_Schema\\11mm_M12

On-the-Fly Project Settings Saving

  • Currently, project-related .blitz files save only on program close, not in real-time. I would like the same as with program settings in "real time"

Dont get your point 7. : I can change settings.blitz in notepad and the changes are used, if they are valid. Corrupt values are ignored. So this is the expected, correct bahaviour.

@irkri
Copy link
Collaborator

irkri commented Nov 12, 2024

Project Saving Checkbox

Done.

Colormap Default Setting

I searched a long time, but can't find a point to get into the pyqgraph library for that. There is no method I can override and no signal I can connect that triggers only when the colormap is changed. Also, the name of the colormap is not registered. It is given to the GradientEditorItem in the HistogramLUTItem of the HistogramLUTWidget, but then directly transformed into the LUT (a tuple of colors and corresponding levels).
Maybe there is a chance to get the name of the colormap directly by accessing the user click on the UI, but I don't know where to find this.

Subset and Size Ratios

The way I designed the settings currently prohibits overriding standard settings with project settings. This would involve connecting and disconnecting signals every time the settings change. For example, if a project file switches, all old connections have to be cut, both from the settings.blitz and the current_project.blitz. This would be again alot of work.

What I did now:
I restructured the project settings. If a project file is loaded, there are some pre-loading settings loaded and corresponding signals connected (here: subset_ratio, size_ratio). Then the data is loaded (with mask, crop). Afterwards, all other settings are loaded (like flipping, transposing, ...).
This way, subset_ratio and size_ratio are now only connected to the project files.

BUG: File Drag-and-Drop Limitation

I don't have that bug. I do everything with drag and drop and it works perfectly fine. Please try to reproduce it by slowly changing project settings. At which setting does the bug happen?

On-the-Fly Project Settings Saving

Everything except for masking and cropping gets saved in real time, also for project settings.
I now changed it so that mask and crop are also saved, this was not easy.
I also noticed a bug along the way, that will be hard to fix: If an already masked dataset is loaded, then masking it again will override the old mask. The next time the dataset is loaded, it is only masked by the new mask. This happens because BLITZ has no recollection of the actual image size if the already masked dataset is loaded.

Dont get your point 7. : I can change settings.blitz in notepad and the changes are used, if they are valid. Corrupt values are ignored. So this is the expected, correct bahaviour.

I meant that you cannot change the path of the settings.blitz. It will always be the same file in the same folder as BLITZ.exe.

@PiMaV
Copy link
Owner Author

PiMaV commented Nov 21, 2024

Colormap Default Setting

I was reffering to viewer.py Line 140:

def auto_colormap(self) -> None:
    if (min_ := self.image.min()) < 0 < (max_ := self.image.max()):
        max_ = max(abs(min_), max_)
        min_ = - max_
        self.ui.histogram.gradient.loadPreset('bipolar')
        self.setLevels(min=min_, max=max_)
        self.ui.histogram.setHistogramRange(min_, max_)
    else:
        self.ui.histogram.gradient.loadPreset('greyclip')
        super().autoLevels()

I would simply like to have a variable in our settings.ini that stores the "gradient" name. So if i prefer to use "plasma" as my default, its set everytime i start the BLITZ

Subset and Size Ratios

This needs to be discussed and tested then. Yet sound functional

BUGS and Savings

Discussion in Person.

I meant that you cannot change the path of the settings.blitz. It will always be the same file in the same folder as BLITZ.exe.

Understood. With the new .blitz endings thats totally fine!

@PiMaV
Copy link
Owner Author

PiMaV commented Nov 21, 2024

  • colormap in init file
  • keep as is for the moment (suubset and size)
  • drag and drop testen

irkri added a commit that referenced this issue Nov 22, 2024
@irkri
Copy link
Collaborator

irkri commented Nov 22, 2024

Colormap:
I managed to finally find a solution for changing the colormap in the settings.blitz file on the fly. The workaround I used is very very dirty programming and involves changing class attributes of the underlying GradientEditorItem. It works, but its not nice.

@PiMaV
Copy link
Owner Author

PiMaV commented Nov 22, 2024

Your choice: either branch, Keep or drop. If it works, it works 🤷🏼‍♂️

@irkri
Copy link
Collaborator

irkri commented Jan 3, 2025

Managed to get back to it now. I decided to keep it.
However, a big problem there was that changing the colormap is interacting with the "Auto Colormap" checkbox. Once the user changes the colormap manually, the checkbox should be disabled. This works now and involved another minor workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion needed first High Prio Needs to be adressed
Projects
None yet
Development

No branches or pull requests

2 participants