Skip to content

Conversation

@MartinDRChacon
Copy link

@cameronwhite
Copy link
Member

Very cool, thank you for working on this!

I likely won't have time until later this week to review more closely, but one initial thought is that maybe we can reuse / extend the existing PerlinNoise.cs to be reusable here?

@MartinDRChacon
Copy link
Author

I think both algorithms aren't compatible. The one used by Caustics effect hat Three coordinates and is seeded. On the other hand, PerlinNoise has only two coordinates (PointD) and is static. But I removed some redundant methods.

focus: 0.1 + Data.Amount,
seed: Data.Seed,
turbulence: Data.Turbulence,
background: palette.PrimaryColor.ToColorBgra (),
Copy link
Member

Choose a reason for hiding this comment

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

I think this likely also should be converting the ColorBgra to premultiplied alpha since the Cairo ImageSurface pixels are premultiplied (see #1553 - it's very easy to get this wrong currently)

The algorithm may also need some adjustments for this - right now the brightness parameter makes the pixels more opaque if the palette color is transparent.

Copy link
Author

Choose a reason for hiding this comment

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

bf4cbff - Please refer to this commit. That is where I tried to address this. :)

[Caption ("Dispersion"), MinimumValue (0), MaximumValue (1)]
public double Dispersion { get; set; } = 0.0;

[Caption ("Time (for animation)"), MinimumValue (0), MaximumValue (100)]
Copy link
Member

Choose a reason for hiding this comment

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

This label feels a bit confusing to me since we don't have a way to create an animated effect out of this.
Maybe something like Time Offset or just Offset? To indicate that it's shifting the noise pattern around

Copy link
Author

Choose a reason for hiding this comment

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

I just changed the caption and property to TimeOffset.

private readonly double[] gradient_z = new double[GRADIENT_SIZE];

// The constructor now takes a seed to generate a unique, repeatable noise pattern.
public ClassicNoise (int seed)
Copy link
Member

Choose a reason for hiding this comment

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

Naming-wise, perhaps something like PerlinNoise3D? I worry that ClassicNoise might be too generic

Copy link
Author

Choose a reason for hiding this comment

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

I just changed the name :)

@cameronwhite
Copy link
Member

Thanks for the changes!

I think there still is something off with the alpha handling. For example, if I have the foreground color set to FF000032 in the color picker (which is unpremultiplied, the premultiplied color would be 32000032, and have the brightness set to 100, then it produces many fully opaque white pixels FFFFFFFF. I would have expected the result to be something like (premultiplied) 32323232 to have white pixels with the same alpha

@MartinDRChacon
Copy link
Author

In the previous commit, the waves always had an alpha value of 255. If I understand correctly, you want the waves to have the same transparency as the background color, but this would make it harder to draw the waves over the picture like in the following example:

img2

The changes were made in the following commit: [f355c87]

But they were reversed in commit: [463c759]

You could accept or discard the last commit. I am open to any other feedback or question. :)

@cameronwhite
Copy link
Member

Thanks :) I agree that behaviour would be useful, but perhaps there could be another option or way of achieving it?
I just think it's very surprising if a brightness control affects opacity - e.g. having to set your primary color to transparent in order to get opaque white caustics on a transparent background doesn't seem the most intuitive

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.

2 participants