Skip to content
This repository was archived by the owner on Jun 9, 2023. It is now read-only.

Comments

Add "Filling" Setting#39

Merged
gregorni merged 16 commits intofsobolev:masterfrom
gregorni:fill-setting
Feb 4, 2023
Merged

Add "Filling" Setting#39
gregorni merged 16 commits intofsobolev:masterfrom
gregorni:fill-setting

Conversation

@gregorni
Copy link
Collaborator

@gregorni gregorni commented Feb 3, 2023

Also merged Wave & Line mode into one and changed some setting descriptions.

Also merged Wave & Line mode into one
@gregorni
Copy link
Collaborator Author

gregorni commented Feb 3, 2023

@fsobolev Some of the modes look rather shitty with line thickness set to 40, even in fullscreen. Do you think I should lower the maximum it to 30, or keep it at 40?

Also, what do you think in general about the rest of this PR? I have the feeling it'll mess up (or get messed up by) #35 .

@gregorni gregorni requested a review from fsobolev February 3, 2023 22:07
@fsobolev fsobolev linked an issue Feb 4, 2023 that may be closed by this pull request
@fsobolev
Copy link
Owner

fsobolev commented Feb 4, 2023

Some of the modes look rather shitty with line thickness set to 40, even in fullscreen. Do you think I should lower the maximum it to 30, or keep it at 40?

There are tons of ways to make Cavalier look like shit. The question is can it look good with the setting in discussion? When thickness maximum value was increased, I said that in small window line mode with 40 thickness looks kinda like jumping cloud, and tbh I find this not very good looking, but okay, maybe somebody will like this. So I don't mind to keep it 40. But I will not insist if are sure it should be 30.

Also, what do you think in general about the rest of this PR?

Love it ❤️ I was worried that modes list can become too long if a few more modes will be added, and you not only resolved this problem (at least temporary) but also made new feature to work with all modes!

I have the feeling it'll mess up (or get messed up by) #35 .

I will make required changes in #35 when this PR will be merged, I will add fill-circle setting to control whether the inner circle should be filled instead of using settings that are currently implemented there for Wave and Bars modes.

@fsobolev
Copy link
Owner

fsobolev commented Feb 4, 2023

Ah yeah, one more change. I think the offset should be increased with thickness, so when thickness is increased the space between elements keeps the same.

@gregorni gregorni changed the title Add "Filled Elements" Setting Add "Filling" Setting Feb 4, 2023
@fsobolev
Copy link
Owner

fsobolev commented Feb 4, 2023

Looks like I fixed everything, feel free to merge if you don't want to change anything ☺️

@gregorni
Copy link
Collaborator Author

gregorni commented Feb 4, 2023

Wait, I've still got some stuff to add, I'm still working on it.

Also updated a description
step = width / ls
offset_px = step * offset / 100
if not fill:
thickness = thickness * (100 - offset) / 100
Copy link
Owner

Choose a reason for hiding this comment

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

I disagree with this change. I already made elements change depending on thickness (see lines 90-99 for example, when fill is off y coordinate and height are calculated taking thickness into account).

Copy link
Owner

Choose a reason for hiding this comment

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

Thickness is set in pixels, so it's unexpected if it will change depending on something else.
But I like how in the last commit you changed in bars mode so when fill is on it just change thickness to 0 and one function is used no matter what in what state fill is, shorter and cleaner code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem I was trying to fix was that if you put thickness to full, and then increased offset, you would see the blank space inside the element shrink to nothing, until it at some point grows again slightly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kooha-2023-02-04-17-10-45.webm

Like this.

Copy link
Owner

Choose a reason for hiding this comment

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

The problem I was trying to fix was that if you put thickness to full, and then increased offset, you would see the blank space inside the element shrink to nothing, until it at some point grows again slightly

I see another symptoms, elements start to grow and overlap eachother if both thickness and offset are big enough.
We need to fix it in some different way, without modifying thickness when the problem is not happening.

Copy link
Owner

@fsobolev fsobolev Feb 4, 2023

Choose a reason for hiding this comment

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

I found a fix for particles mode, I just limited thickness to not be more than half of minimum size of an element. I assume the same should work for other modes, going to implement it all and commit then.

EDIT: ah, damn, no, the issue from your video still can be reproduced. I guess offset needs to be limited too somehow.

Copy link
Owner

Choose a reason for hiding this comment

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

Is it really so bad to use offset?

Yes. Thickness is in pixels. If a user set it to be 20, it should be 20, unless it causes problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you try again with my latest commit? It fixed it for me, but thickness calculation includes step & offset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it really so bad to use offset?

Yes. Thickness is in pixels. If a user set it to be 20, it should be 20, unless it causes problems.

Well, we could also change the description of thickness... 😬

Copy link
Owner

Choose a reason for hiding this comment

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

Well, we could also change the description of thickness... grimacing

Or we can make it work properly :) I found a way to fix particles mode when roundness is 0, but when I increase roundness it doesn't work for some reason. I'm goind to find why and fix it

Bars also scale now when window is resized
fills/strokes less often now
@gregorni
Copy link
Collaborator Author

gregorni commented Feb 4, 2023

@fsobolev Is there anything else you disagree with?

@gregorni gregorni requested a review from fsobolev February 4, 2023 16:55
@fsobolev
Copy link
Owner

fsobolev commented Feb 4, 2023

Kooha-2023-02-04-20-16-44.webm

@fsobolev
Copy link
Owner

fsobolev commented Feb 4, 2023

Thickness is now limited in all modes. If you toggle between fill and stroke, no matter what are values of thickness and offset, you get the same shape. Roundness changes it if it's not exactly 0 or 1, because with increased thickness the actual size of element is changed and in result it's not rounded the exact same way, but I really don't think it's an issue, since basically the size is the same.
If you are fine this this, feel free to merge.

@gregorni gregorni merged commit 6c6f245 into fsobolev:master Feb 4, 2023
@gregorni gregorni deleted the fill-setting branch February 4, 2023 18:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge Line & Wave into one

2 participants