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: added support to border and radius for canvas2d #425

Merged
merged 15 commits into from
Nov 7, 2024

Conversation

pecoram
Copy link
Contributor

@pecoram pecoram commented Oct 31, 2024

No description provided.

@pecoram pecoram requested a review from philippe-wm November 4, 2024 08:46
@pecoram
Copy link
Contributor Author

pecoram commented Nov 4, 2024

there is a problem with the position of the border... on the webgl renderer the border is inside (which I think is wrong or at most to be made configurable) while on the canvas renderer it is centered

@pecoram pecoram marked this pull request as ready for review November 5, 2024 09:07
@pecoram pecoram requested a review from elsassph November 5, 2024 10:50
@wouterlucas
Copy link
Contributor

Nice, thanks for the contribution! I like how you've isolated it in the Canvas implementation and thanks for the extended review @elsassph!

Heads up @jfboeve has been working on a redesign on the shaders/effects portion of the renderer. Stepping away from the Dynamic Shader and having tree shakeable shaders.

One of the topics we discussed is shaders and Canvas2D and we're keeping those separate. Limiting Effects to WebGL1/WebGL2 only as it does do not make sense to combine. Trying to come up with something to caters to both WebGL & Canvas2D would only lead to overly complicated effects definition for only a small number of applicable effects for Canvas2D anyway.

We'll have to refactor this PR a little bit to make it fit with the new effects/shaders structure down the road.

@pecoram pecoram requested a review from elsassph November 6, 2024 08:59
if (shType !== ROUNDED_RECTANGLE_SHADER_TYPE) {
console.warn('Unsupported shader:', shType);
}
// if (shType !== ROUNDED_RECTANGLE_SHADER_TYPE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should that be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's decide, having added the missing “shaders” I think it is no longer necessary. In addition, this warning makes the inspector unusable

@wouterlucas wouterlucas added this pull request to the merge queue Nov 7, 2024
Merged via the queue into lightning-js:main with commit efa9ed9 Nov 7, 2024
2 checks passed
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.

4 participants