lbalazscs / Pixelitor

A desktop image editor
https://pixelitor.sourceforge.io/
GNU General Public License v3.0
181 stars 70 forks source link

Improved Truchet Filter #347

Closed Minecraftian14 closed 4 months ago

Minecraftian14 commented 6 months ago

Idea -> Editable Swatches Explanation -> Instead of having a set of predefined types parameter in the filter menu, we had a decision that it will be a nice idea to have a complete editor for creating your own types.

For the scope of this PR,

Features I dreamt of:

Edit: 26-02-2024

Edit: 29-02-2024 https://github.com/lbalazscs/Pixelitor/pull/347#issuecomment-1971328833

https://discord.com/channels/887584993994481664/887584993994481667/1212795558708908062

https://discord.com/channels/887584993994481664/887584993994481667/1212796142283268147

Edit: 03-03-2024 https://discord.com/channels/887584993994481664/887584993994481667/1213853035785232415

LinuxBeaver commented 6 months ago

I'm just posting this to congratulate you on your hard work.

lbalazscs commented 6 months ago

Great work! I plan to write more general review in Discord, these are just some technical observations:

  1. Let's not sacrifice too much in order to maintain compatibility with the old Truchet filter. We could keep the filter from the last release as it was, and also have a separate, new filter (exactly how "NeoTruchet" is separate from "Truchet" now) without compatibility constraints. The old, "legacy" filter doesn't have to be in the menus, simply keeping it in the code should make sure that old smart filters continue to work (presets would still break, but that's not so bad). This would also prevent the 'French numbers' to 'gemstone names' migration from running with every preset load. So don't worry about compatibility.

  2. The filter has an issue that only happens when the -ea and the -Dpixelitor.development=true command-line options are given (which is always a good idea, because they enable extra runtime correctness checks). I think the reason is that recently I made some changes in the way the filters are started. Now, if a filter has a dialog, the filter starts running only after the dialog becomes visible. Although I don't fully understand why the assertion error is happening, since NeoTruchet is a ParametrizedFilter, and in theory all standard parametrized filters should just work, because these things should be handled at the superclass level. I've had to fix similar problems recently in non-standard filters like Levels and Curves. I suppose it must be related to the TruchetParam.

  3. It would be good to add a line sometime to the FilterParamTest to confirm that the new TruchetParam follows the "FilterParam contract". Right now, this would result in 6 failing tests. Perhaps fixing these would also solve the problem with the runtime assert error. Please let me know if you have any questions related to this.

Minecraftian14 commented 6 months ago

Let's not sacrifice too much in order to maintain compatibility

Sure, that's a really great idea!

The filter has an issue that only happens when the -ea and the -Dpixelitor.development=true

That was likely to happen... I had to make my own classes for the GUI and the Param. All I was following was RangeParam - which was sufficiently large enough to confuse me. The end result, my Param and GUI are now stringly coupled, I even call params changed event inside the GUI constructor. That's most likely what's causing the problem. At the moment, I am not really interested in fixing that issue since I'll have to rewrite the param code and well - I'll have to study other existing Param codes. But yes, I'll fix that in future commits.

It would be good to add a line sometime to the FilterParamTest

I understood nothing. I'll check out the test cases and try to figure out this concept. I'll ask you my doubts after I try to understand it more.

Thanks ✨

Minecraftian14 commented 6 months ago

The last commit is crazy 👀! I still can't believe that works!!! (I am referring to the mouse highlights when related cells are altered)

lbalazscs commented 6 months ago

I tried the newest version, but I don't understand what you mean by mouse highlight, or by altering related cells... By cells you mean the elementary tiles? But then all tiles are related and also independent, and I see no special highlighting effects as I click them in order to change them...

Minecraftian14 commented 6 months ago

I was actually referring to this grey-out effect on the tiles when mouse is moved over the tiles. image

The idea was, that generated tiles create some kind of swarms of different tile types which are related to each other and from editable groups. When a single tile is edited in that group, the whole group changes. This effect is particularly useful when one wants to change the tiles in the generated pattern, but doesn't want to disturb how to tiles look relative to each other.

Next up, I really wanted to implement this feature in this filter - but it's pretty complicated to not only understand, but also to code. I wanted to experiment making some kind of highlighting feature which will be able to highlight all the tiles which will change in case I edit the tile I am hovering my mouse over.

lbalazscs commented 6 months ago

Ah, I see now. In general I'm against things that are hard to understand. I think most people will invest a limited amount of effort trying to understand how this filter works, so anything that requires thoughtful consideration will confuse them, even if it is a powerful idea. Ideally a UI should be discoverable through exploration (by random clicking...), and this can be achieved by minimizing surprises and being consistent. I mean, it's surprising if clicking on a tile changes multiple other tiles. BTW, this is also why I'm not enthusiastic about the symmetry/rotation feature.

Regarding the code, I'm also skeptical about "I still can't believe that works" scenarios, because usually this is the kind of code that is hard to debug. As a famous quote (by Brian Kernighan) says, "Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?".

All this doesn't mean that I won't merge it if you manage to make it bug-free and reasonably usable, even if it's hard to understand. But my advice would be to make things simple and consistent, even if that means sacrificing some interesting ideas.

lbalazscs commented 4 months ago

I'm closing this pull request due to lack of activity. Please feel free to reopen it or create a new one if there are any updates.