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

GLES2 polyline drawn as GL_LINE_STRIP to match GLES3 #38734

Merged
merged 1 commit into from
May 14, 2020

Conversation

lawnjelly
Copy link
Member

The behaviour of TYPE_POLYLINE appeared incorrect in GLES2, and inconsistent with GLES3 and the docs, which state that draw_polyline 'Draws interconnected line segments'. Also when drawing with triangles GLES2 draws interconnected segments.

This PR simply changes the primitive from GL_LINES to GL_LINE_STRIP as in GLES3.

Fixes #38703.

Notes

Although this is 'correct', we should also consider that it is a breaking change, that may affect people who have been relying on the bugged behaviour. Do we have a drawlines methods they can change to that does multiple line segments?

It seems Item::Command::TYPE_LINE is for single lines. Maybe we could have some kind of variation of draw_polyline that allows separate segments?

The behaviour of TYPE_POLYLINE appeared incorrect in GLES2, and inconsistent with GLES3 and the docs, which state that draw_polyline 'Draws interconnected line segments'. Also when drawing with triangles GLES2 draws interconnected segments.

This PR simply changes the primitive from GL_LINES to GL_LINE_STRIP as in GLES3.
@lawnjelly lawnjelly requested a review from reduz as a code owner May 14, 2020 09:48
@akien-mga akien-mga added this to the 3.2 milestone May 14, 2020
@Calinou
Copy link
Member

Calinou commented May 14, 2020

Do we have a drawlines methods they can change to that does multiple line segments?

Yes, draw_multiline/draw_multiline_colors should do it.

@lawnjelly
Copy link
Member Author

Do we have a drawlines methods they can change to that does multiple line segments?

Yes, draw_multiline/draw_multiline_colors should do it.

Ah, it seem to, you are right, there is a multiline if section in the polyline. The docs are confusing on this to me:

void draw_multiline(points: PoolVector2Array, color: Color, width: float = 1.0, antialiased: bool = false)

Draws multiple, parallel lines with a uniform color. width and antialiased are currently not implemented and have no effect.

I'm not clear on where the parallel bit comes in...

@akien-mga
Copy link
Member

There are various issues with draw_polyline/draw_multiline: #16448, #16466, #35878.
Their documentation should also not be taken as reference, it's an educated guess from documentation writers of what this be used for, based on what it actually does and what the code seems to do.

Fixing bugs that would theoretically break compat like here is OK IMO, I don't think we can consider that the previous invalid behavior was a feature/supported.

@akien-mga akien-mga merged commit f5a3d34 into godotengine:3.2 May 14, 2020
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants