Add styling possibilities to StyleBoxFlat + poly count optimization

[davthedev]

Borders can be styled using separate colors, thus allowing effects such as beveled buttons or partial frames

See godotengine/godot-proposals#6910

This is a partial rewrite of the StyleBox drawing code. Includes:

• Addition of simple gradients to the border and fill (2-point, strictly horizontal or vertical)
• Addition of bevel and inset modes - it's just a different way to calculate border colors according to a simulated light source angle, and you get those bevel effects that were present in many of the classic operating systems
• Support for styling the four borders separately
• Choice of how two borders are joined together at the corners (gradient or straight cut; the angle of the latter can be set)
• Reduction of the triangle count. For instance, each rendering of the center fill still had a useless ring of zero-sized triangles, easily removing 72 useless triangles per drawn StyleBox with rounded corners and the default parameters!

I did the trick by vertices coloring only. No shaders & textures involved. Hence the limitation on the orientation & number of color points in gradients.

Actually, to get the gradients, shaders are the ultimate method and the technique is actually used in web browsers. It would allow any orientation, multiple color stops, radials... without having to worry about how to split the rectangle into triangles. But there would be a conflict if the Control node that renders the StyleBox already has a shader applied to it. Unless there is a way of appying 2 shaders on top of each other in such case, which I am not aware of AFAIK.

Properties tree is rearranged, while keeping the same names for the already implemented ones such as the bg color, not to break compatibility with existing projects. This is the reason I could not put it into the new Background subcategory, for instance.

Open for discussion if I missed something!

And there are high chances we can cherry-pick this into Godot 3.

[Calinou]

• Support for styling the four borders separately

I remember seeing this feature in the initial StyleBoxFlat PR, but it was removed when the implementation was changed to use polygons.

But there would be a conflict if the Control node that renders the StyleBox already has a shader applied to it. Unless there is a way of appying 2 shaders on top of each other in such case, which I am not aware of AFAIK.

• See Add a shader property to the StyleBox resource godot-proposals#3494.

[YuriSizov]

This is a lot of different features bundled into the same PR. While rendering improvements are likely welcome, the demand for new features is debatable and needs to be evaluated individually. It's also problematic that this PR introduces conflicting properties (such as two ways to set the border color). Yes, we can't break compatibility, but this is bad API design and is not user-friendly.

For gradients specifically, if this feature is needed, we should use our existing gradient resources instead of an ad hoc and very limited re-implementation.

Overall, my recommendation is to limit the scope and work on this in steps, starting with rendering improvements.

[davthedev]

Thanks for the feedback. This opens up a lot of questions and I would like some guidance on how to do those things a bit more like you suggest.
We can discuss in the related proposal, definitely!

Interdependency of the features

Some of the features depend on the changes implemented for others.
For instance, setting border gradient is built on top of setting the four border colors separately.
Thus, I would have to wait for the first one to be discussed and merged before proposing the second one and so on. Is there a way to avoid this bottleneck and comply with the idea to separate the features?

Balance between good API design and backwards compatibility

I made a compromise in my proposal between the two, whis is not perfect. How would you change it, better consistency at the expense of backwards compatibility? Or, perhaps, some features of the property system that may allow using the old property identifiers while showing different names for them in the editor I may not be aware of?

The gradient algorithm

The proposed implementation is a bit less complete in possibilities, but has a performance bonus because it does not need a texture. In the case of a StyleBox that can be frequently drawn for buttons, frames, progress bars... for instance, it can be an advantage. I would say that the simplified algorithm covers most use cases, and we may add the possibility to use the texture for advanced ones. My initial thought was to internally use a shader, but I am stuck with the possible conflicts.

If I use a GradientTexture2D to apply the gradient instead, I need to specify a fixed rendering size for it. It can be problematic if the StyleBox is larger as rendering artifacts can appear.

The way to set borders:

I thought of the possibility to

• set an overall style for all four edges
• override the setting for specific edges for more customization.

It maintains the possibility to fully customize the edges, while providing some shortcuts to try out styles without having to set up the four border colors separately. Initially I added the possibility to style each separate border only, and I found out that it is tedious just to try out different gradient colors.
Same goes for the bevel effect. It does the color mixing for you and applies it to the edges.

Would you suggest a different approach?

[YuriSizov]

Thus, I would have to wait for the first one to be discussed and merged before proposing the second one and so on. Is there a way to avoid this bottleneck and comply with the idea to separate the features?

That's right, you would have to wait for the work to be approved. That's the whole point of splitting your work, to remove inherent interdependency and make changes in sensible steps, discussing and evaluating each of them. Otherwise you put us in a position where we have to accept or reject everything at once. And in that case, this should be rejected, because it contains a lot of changes massively extending the API surface without a lot of evidence that there is a demand for it.

Smaller, clearly beneficial changes are better than betting everything at once.

How would you change it, better consistency at the expense of backwards compatibility?

Well, to discuss that we would need to see which features are actually needed and how their use-cases look like. All the planning would start there. As an arbitrary suggestion, a new stylebox type can be added. In fact, it can probably be added as an addon first, and then we would see if it's niche or if it's something most users would benefit from.

In the case of a StyleBox that can be frequently drawn for buttons, frames, progress bars... for instance

Nothing is drawn more frequent than it is updated, and a few textures shouldn't be an issue for a game engine. If there are some reasonable performance benefits, they need to be benchmarked and provided. But you also don't have to use a GradientTexture. I was talking about the Gradient resource, a type that holds a collection of colors and some interpolation information, and it also has an inspector plugin for easy editing. How you'd use that information is up to your implementation, but at least we wouldn't be sacrificing usability.

But that can be discussed in a dedicated PR in more detail.

[davthedev]

Let's go for splitting the features and proposing smaller PRs. Makes sense.

How could something as massive as the tilemap system rewrite be approved if everything should be done in small iterations only? How do you conduct reviews for reworks that require syncing the progress on multiple parts together?

Because I see some cases to be better implemented all at once, especially if an API is changing, so that there is only one moment where the backwards compatibility breaks instead of multiple successive ones.

Also, let's see the following situation.

• I implement feature A for proposal
• I implement feature B for proposal independently, Don't want to wait for A to be merged. So I copy part of the logic of feature A which is useful for B to have the discussion started on it.
• Feature A gets accepted and merged.
• Now there is a conflict with feature B, I have to resolve it (the main thing I want to avoid).

In software engineering, I find it much easier to plan well rather than handle conflicts.
On the other hand, implementing an empty shell "just in case" by thinking of the other features in separate PRs could be interpreted as dead code in the scope of the related PR. How to strike a good balance according to the project governance?

[Calinou]

How to strike a good balance according to the project governance?

I strongly recommend joining the Godot Contributors Chat and participating in relevant channels if you're working on large-scale features. Real-time chat makes it easier to coordinate on this.

[davthedev]

Thanks! Just found out all the feature-specific channels there.
I visited the chat before but the place to find channels other than new-contributors and devel is not straightforward IMHO. Perhaps drop a note in the docs about that?

[YuriSizov]

How could something as massive as the tilemap system rewrite be approved

It was discussed and approved before the work has even started, whereas you're coming with an implementation first. You made the proposal the same day, there was no discussion, no consensus on the feature set, no opinions of stakeholders or users shared to guide you to the most viable, most useful implementation. You say

In software engineering, I find it much easier to plan well rather than handle conflicts.

And this is why we ask everyone to follow the proposal process, submit an idea first, and through discussion to arrive to a path that makes sense and doesn't waste your time. We do not advise anyone to work on a feature that hasn't been discussed first. It's just a bad idea, unless you're very much aware of the possibility and ready for your work to be rejected.

Also, let's see the following situation.

In this situation you would include the commit from PR A into PR B, mark B as a draft, and be ready to rebase it once A is merged. You wouldn't include changes from A into the same commit as changes from B. That's actually a pretty normal thing many contributors do, submitting connected PRs where one contains commits from the other, ready to be rebased.

And once again, "to have a discussion started" you open a proposal, not a PR. PR is the final step of the feature implementation, even though it's often tempting to jump ahead or just arrive to an implementation through experimenting and creating a PoC.

On the other hand, implementing an empty shell "just in case" by thinking of the other features in separate PRs could be interpreted as dead code in the scope of the related PR.

You should not implement anything unrelated to the PR, not unless you've been approved ahead of time to build a framework for future expected changes. In this case you have rendering optimizations and you have several new features. Features may depend on your changes to the rendering code, but that doesn't mean that you should open a PR making rendering optimizations and adding some "empty shell" for the features. Your PR should only include optimizations, and the rest of the changes should come with their corresponding features in dedicated PRs.

[davthedev]

It was discussed and approved before the work has even started, whereas you're coming with an implementation first. You made the proposal the same day, there was no discussion, no consensus on the feature set, no opinions of stakeholders or users shared to guide you to the most viable, most useful implementation.

I opened some proposals about that change, knowing that all features require discussion, exactly for the reasons you mention. There are two of them - the one specific to the new features of this PR was very young, and a more general one with some screenshots that was opened before.

Definitely I prefer to have an open discussion and get those opinions.

Here are the related proposals.
godotengine/godot-proposals#6910
godotengine/godot-proposals#6812

Perhaps the links were not mentioned clearly enough in the description.

even though it's often tempting to jump ahead or just arrive to an implementation through experimenting and creating a PoC

Making a PoC is how I can evaluate the technical feasibility in many cases and quickly see if there are corner cases or quirks that would make the idea not practical. And, if it works, I can visually experiment with its results and see what can be created with it, explain it with screenshots that are 1:1 accurate with what you get.

The code here is an "expanded proof of concept" indeed, and I thought the process of collaborating on the PR would be a good place to make the reviews to turn it into something suitable for our quality standards, with the corresponding discussion about features in the attached proposal.

Your PR should only include optimizations, and the rest of the changes should come with their corresponding features in dedicated PRs.

It's done now. For the optimizations only, see #77278
For the features, it's back to the drawing board for more proper discussion!