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

fix drawLine wrong when thickness!=1 #2327

Closed
wants to merge 1 commit into from

Conversation

NgVThangBz
Copy link
Contributor

@NgVThangBz NgVThangBz commented Jan 11, 2025

fix drawLine with thickness other than 1
factor = (FrameSize /designResolutionSize)*2
and the smallest thickness that axmol can create is 1,
so in all cases where the thickness is <1 I think we should draw a line with thickness = 1

See detailed error at #2324

Checklist before requesting a review

For each PR

  • Add Copyright if it missed:
    - "Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."

  • I have performed a self-review of my code.

    Optional:

    • I have checked readme and add important infos to this PR.
    • I have added/adapted some tests too.

For core/new feature PR

  • I have checked readme and add important infos to this PR.
  • I have added thorough tests.

@aismann
Copy link
Contributor

aismann commented Jan 11, 2025

@NgVThangBz
Please move this to the constructor of DrawNode.cpp

        _factor2 = 2 * AX_CONTENT_SCALE_FACTOR(); 

DrawNode .h:
float _factor2;

Maybe this is better:

    float width =  thickness / _factor2;
    Vec2 nw = n * width ;
    Vec2 tw = t * width ;

Please also add a cpp tester which test different factors or at least the
correct draw of Label->enableUnderline();

@aismann
Copy link
Contributor

aismann commented Jan 11, 2025

@NgVThangBz
If you give me some more time I will make a PR for this issue (So you can close your PR when you want)
Currently writing the cpp-tests tester for at least Label->enableUnderline();
image

@rh101
Copy link
Contributor

rh101 commented Jan 12, 2025

@NgVThangBz Please move this to the constructor of DrawNode.cpp

        _factor2 = 2 * AX_CONTENT_SCALE_FACTOR(); 

@aismann If the content scale factor is changed at runtime (such as due to screen resolution changes), then the above cannot be in the constructor. If it changes after the DrawNode is created, the resulting output will be incorrect.

@NgVThangBz
Copy link
Contributor Author

@aismann factor != AX_CONTENT_SCALE_FACTOR()
factor = 2*FrameSize()/DesignResolutionSize()

    float fS = Director::getInstance()->getGLView()->getFrameSize().width;

    float dS = Director::getInstance()->getGLView()->getDesignResolutionSize().width;

    float factor = 2 * fS / dS;

and as @rh101 mentioned, if set in constructor it will not be accurate

@rh101
Copy link
Contributor

rh101 commented Jan 12, 2025

@NgVThangBz It may be better to do as @aismann suggested, which is to close this PR and let him submit one. The reason is that @aismann is very familiar with the DrawNode implementation, and a change like this has a wide impact, so it may require more test cases, which you have not added to this PR.

@aismann
Copy link
Contributor

aismann commented Jan 12, 2025

@aismann factor != AX_CONTENT_SCALE_FACTOR() factor = 2*FrameSize()/DesignResolutionSize()

    float fS = Director::getInstance()->getGLView()->getFrameSize().width;

    float dS = Director::getInstance()->getGLView()->getDesignResolutionSize().width;

    float factor = 2 * fS / dS;

and as @rh101 mentioned, if set in constructor it will not be accurate

@NgVThangBz
cpp-tests use '*.height' as input values:
director->setContentScaleFactor(g_resourceSize.height / g_designSize.height);

You switch to "width":

float fS = Director::getInstance()->getGLView()->getFrameSize().width;
float dS = Director::getInstance()->getGLView()->getDesignResolutionSize().width;

What is your reason to use 'width' instead of 'height'?

I think your solution is sometimes not a correct value because the factor between
getFrameSize().width / getDesignResolutionSize().width
and
getFrameSize().height/ getDesignResolutionSize().height
can be totaly different.
I belive define/changing the factor is part of the developer.
And DrawNode should add a getFactor/setFactor API and a default value = (AX_CONTENT_SCALE_FACTOR()*0.5f).

@NgVThangBz
Copy link
Contributor Author

I will close my PR, and @aismann will continue

@NgVThangBz NgVThangBz closed this Jan 13, 2025
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