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

Correct odd layer GUI description #5793

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

vovodroid
Copy link
Contributor

Currently every second layer for "on odd layer" options is checked as layer_id % 2 == 1. Though from mathematics POV it's correct, but in GUI first layer is one and not zero, thus actually option is applied for even layers.

I guess this is SuperSlicer legacy, and this PR fix this confusing description in GUI.

image

image

@kisslorand
Copy link
Contributor

Isn't the first layer numbered "0"?

@vovodroid
Copy link
Contributor Author

vovodroid commented Jun 21, 2024 via email

@vovodroid
Copy link
Contributor Author

image

@vovodroid
Copy link
Contributor Author

Another option - call it "every other layer", like here

image

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

Nice catch!
Can we change the code logic to make it align with the UI, but leave the text/description unchanged?
e.g.
change code from layer_id % 2 == 1; to layer_id % 2 == 0;

@vovodroid
Copy link
Contributor Author

Sure we can, but layer_id % 2 == 0 includes first (zero) layer, thus demanding special handling. Also is change perimeters for all existing projects. So I guess change description is much better.

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

The catch with changing user facing text is that it will invalidate all existing translations

@vovodroid
Copy link
Contributor Author

Translations could be updated, it's updated on GUI changes anyway.
Also expressions layer_id % 2 == 0 will affect first layer, and layer_id > 0 && layer_id % 2 == 0 will skip two first layers, causing inconsistency in behavior.

@vovodroid vovodroid force-pushed the odd-layer-description-pr branch from 9f15b35 to 2689e9e Compare September 9, 2024 07:47
@SoftFever
Copy link
Owner

Translations could be updated, it's updated on GUI changes anyway. Also expressions layer_id % 2 == 0 will affect first layer, and layer_id > 0 && layer_id % 2 == 0 will skip two first layers, causing inconsistency in behavior.

Good point :)

Copy link
Owner

@SoftFever SoftFever left a comment

Choose a reason for hiding this comment

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

LGTM

@SoftFever SoftFever merged commit 09ffca1 into SoftFever:main Sep 9, 2024
15 checks passed
@vovodroid vovodroid deleted the odd-layer-description-pr branch September 9, 2024 16:41
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.

3 participants