Allowing user to choose gradients from different sources, and allowing `CloudsEffect` to make use of custom gradients by Lehonti · Pull Request #683 · PintaProject/Pinta

Lehonti · 2024-01-13T13:35:48Z

Besides the pre-defined ones, the user can choose to get them from the colors that are currently selected (like it's done in CloudsEffect or to randomize the colors and the stops).

Also, modified CloudsEffect to be able to use custom gradients. Here's a rendering of clouds with the Electric color scheme:

...but at the same time the defaults should make it behave the same way it always has.

…d colors

cameronwhite · 2024-01-13T16:23:50Z

+				return CreateColorGradient (colorScheme);
+			case ColorSchemeSource.SelectedColors:
+				return ColorGradient.Create (
+					PintaCore.Palette.PrimaryColor.ToColorBgra (),


Rather than accessing PintaCore.Palette, the caller should provide an IPaletteService.
For unit tests, this avoids initializing a ton of things in Pinta.Core that end up failing (this is one of the reasons why the clouds effect didn't have a unit test already)

Instead of accessing `PintaCore.Palette`

Lehonti · 2024-01-13T16:58:42Z

Thanks for the feedback. I've now changed it so that an IPaletteService is passed as an argument

…bal palette is removed

Lehonti · 2024-01-13T17:07:13Z

Also, I added IgnoreAttribute to some tests while the dependency on the global palette is still there

… the global palette is removed" This reverts commit 71fc127.

cameronwhite · 2024-01-13T17:09:20Z

Also, I added IgnoreAttribute to some tests while the dependency on the global palette is still there

We should remove that dependency so we can test them :)
I think the effect needs to be constructed with an IPaletteService. I'm not sure if we do that for any effects yet, but the tools are a good example of doing that. And then the unit tests can create a mock palette with the colours to test with

Lehonti · 2024-01-13T17:11:37Z

I removed the IgnoreAttributes, and got the palette to be accessed lazily while this is done. This has the added advantage that we can now test the CloudsEffect

… should be now be testable)

Lehonti · 2024-01-13T18:12:21Z

These effects are now being passed an IServiceManager to the constructor, and there is now a mock palette :)

(We should probably pass IServiceManager to the constructor of other effects, but since they aren't part of the public API, I don't think it's urgent, and it may be for a future pull request, also for keeping the pull request bite-sized)

cameronwhite · 2024-01-14T03:02:11Z

 	public CloudsData Data => (CloudsData) EffectData!;  // NRT - Set in constructor

-	public CloudsEffect ()
+	public IPaletteService palette { get; }


Should this have been private?

Yes, it should have been private. Thanks for catching it. It has been corrected now

cameronwhite · 2024-01-14T03:05:47Z


+public enum ColorSchemeSource
+{
+	[Caption ("Predefined Gradient")]


I'd prefer Preset Gradient as the label for the user here

No problem. The label has been changed

cameronwhite · 2024-01-14T03:09:59Z

+internal sealed class MockPalette : IPaletteService
+{
+	public Color PrimaryColor { get; set; } = new (0, 0, 0); // Black
+	public Color SecondaryColor { get; set; } = new (255, 255, 255); // White


I think this is incorrect since Cairo.Color is floating point, so the 255's should be just 1 (look at the ToColorBgra method)
You could also just do ColorBgra.White.ToCairoColor()

You are right. For a moment I forgot that in Cairo the color channels go from 0 to 1. It's corrected now

cameronwhite · 2024-01-14T03:13:40Z

+	public Color PrimaryColor { get; set; } = new (0, 0, 0); // Black
+	public Color SecondaryColor { get; set; } = new (255, 255, 255); // White
+
+	public void SetColor (bool setPrimary, Color color, bool addToRecent = true)


Because the Utilities class is creating a singleton palette, this could be a future source of bugs - if tests try to set the color then this might cause the changes to affect other test runs. (And tests are executed in parallel)
I'd maybe lean towards not caching the mock service manager for now since it's currently very cheap to construct

Understood. It's now creating an instance every time

cameronwhite · 2024-01-14T03:15:45Z

+		mock_services = CreateMockServices ();
+	}
+
+	private static IServiceManager CreateMockServices ()


I haven't tried running the benchmarks with these changes, but will it cause any runtime errors if there isn't a palette provided now?

No, it shouldn't cause any runtime errors. This was added because the constructors of those effects now receive an IServiceManager, but I was careful so that the default behaviors have the exact same result as before the changes.

If the code path that was being followed before did not access the palette, it doesn't access the palette now, either.

I get the following from running dotnet run --project tests/PintaBenchmarks -c Release -- -f \*JuliaFractalEffect

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.ApplicationException: Could not resolve service type Pinta.Core.IPaletteService at Pinta.Core.ServiceManager.GetService[T]() in /Users/cameron/code/Pinta/Pinta.Core/Managers/ServiceManager.cs:line 55 at Pinta.Effects.JuliaFractalEffect..ctor(IServiceManager services) in /Users/cameron/code/Pinta/Pinta.Effects/Effects/JuliaFractalEffect.cs:line 35 at PintaBenchmarks.EffectsBenchmarks.JuliaFractalEffect() in /Users/cameron/code/Pinta/tests/PintaBenchmarks/EffectsBenchmarks.cs:line 96

You are right...The mock palette should be created and added to the service manager first. I just added it; this error you got shouldn't happen again

…ices`

Lehonti added 8 commits January 13, 2024 13:57

Allow user to create random gradients and to create them from selecte…

0b6b497

…d colors

Not transparent for now

04726ed

Using InvLerp for calculation of stop position

d8a9dfd

Using InvLerp with generic applied...otherwise it thinks it's int

a2b07ab

Reformatting

4f1b3f0

Changed parameters

9bdd0ac

Merge branch 'improvement6' into feature/gradient_from_selected

aaf843d

Got clouds to use custom gradient mechanism

b1aad76

Lehonti changed the title ~~Allow user to choose gradients from different sources~~ Allow user to choose gradients from different sources, and allowed CloudsEffect to make use of custom gradients Jan 13, 2024

Lehonti changed the title ~~Allow user to choose gradients from different sources, and allowed CloudsEffect to make use of custom gradients~~ Allowing user to choose gradients from different sources, and allowing CloudsEffect to make use of custom gradients Jan 13, 2024

cameronwhite reviewed Jan 13, 2024

View reviewed changes

Passing IPaletteService

ed122b3

Instead of accessing `PintaCore.Palette`

Added IgnoreAttribute to some tests until the dependency on the glo…

71fc127

…bal palette is removed

Revert "Added IgnoreAttribute to some tests until the dependency on…

8d2d068

… the global palette is removed" This reverts commit 71fc127.

Accessing palette lazily (workaround)

7687756

Lehonti added 6 commits January 13, 2024 18:26

Added test for clouds (with the ability to choose the color scheme it…

7a8b157

… should be now be testable)

Changed clouds test file

96940d9

Removed image for test

f3aa34b

Added mock services so that effects that depend on palette can be tested

bd4a7b3

Refactored services access into a variable

c97a29b

Added mock services to benchmarks

a0ecabc

Lehonti added 4 commits January 13, 2024 20:58

Merge branch 'PintaProject:master' into feature/gradient_from_selected

989c75c

Alpha channels taken into consideration in GradientHelper.RandomColor

d156b63

Moved RandomColor to EffectHelper as an extension

b74a8d6

Moved RandomColor to OtherExtensions

289992a

cameronwhite reviewed Jan 14, 2024

View reviewed changes

Lehonti added 4 commits January 14, 2024 04:30

Implemented suggested changes

eb54d72

Merge branch 'PintaProject:master' into feature/gradient_from_selected

c214a5a

Added IPaletteService to service manager created by `CreateMockServ…

6d8446c

…ices`

Merge branch 'PintaProject:master' into feature/gradient_from_selected

4ccbac0

cameronwhite merged commit d460c63 into PintaProject:master Jan 14, 2024

Lehonti deleted the feature/gradient_from_selected branch January 14, 2024 10:33

Uh oh!

Conversation

Lehonti commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lehonti commented Jan 13, 2024

Uh oh!

Lehonti commented Jan 13, 2024

Uh oh!

cameronwhite commented Jan 13, 2024

Uh oh!

Lehonti commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lehonti commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lehonti Jan 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lehonti Jan 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lehonti Jan 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Lehonti commented Jan 13, 2024 •

edited

Loading

Lehonti commented Jan 13, 2024 •

edited

Loading

Lehonti commented Jan 13, 2024 •

edited

Loading

Lehonti Jan 14, 2024 •

edited

Loading

Lehonti Jan 14, 2024 •

edited

Loading

Lehonti Jan 14, 2024 •

edited

Loading