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

Label text systems #1512

Closed
wants to merge 1 commit into from

Conversation

alec-deason
Copy link
Member

Add system labels to the text renderer. Incremental work towards #1466

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 24, 2021
@alice-i-cecile alice-i-cecile added this to the Bevy 0.5 milestone Feb 24, 2021
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Those sure are some labels. All of the names even line up!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 24, 2021
@alec-deason
Copy link
Member Author

I learn my lessons.

@cart
Copy link
Member

cart commented Mar 3, 2021

I don't think we should go down the "label every system" route until we've decided it is a "best practice". In the short term, I'd prefer it if we only added labels when dependencies are actually needed.

@alice-i-cecile
Copy link
Member

I don't think we should go down the "label every system" route until we've decided it is a "best practice". In the short term, I'd prefer it if we only added labels when dependencies are actually needed.

How do we know which dependencies external users are going to need?

@alec-deason
Copy link
Member Author

What is the cost to eagerly labeling things? This PR (and the other similar one) exists because a user needed to order systems and couldn't. We could avoid having to ramp through that confusion/frustration/escalation cycle each time by front loading the work. Maybe there's some concept of "private" systems that don't need labels because the user should never think about them, but that seems to run contrary to the bevy philosophy of open architecture, as I understand it.

@cart
Copy link
Member

cart commented Mar 4, 2021

How do we know which dependencies external users are going to need?

We don't. It is a matter of encapsulation. There is a reason we don't label every symbol with pub in Rust (which is a close analog to labeling systems). It opens up the doors for users to break guarantees we want to make with our apis (both intentionally and inadvertently). We won't be able to anticipate every need (just like we can't in rust code). But that doesn't mean we should just pub everything.

Theres also the slightly smaller concern of boilerplate, which y'all know my opinion on.

This PR (and the other similar one) exists because a user needed to order systems and couldn't. We could avoid having to ramp through that confusion/frustration/escalation cycle each time by front loading the work.

I think we should try to understand each use case / requirement, then build a public api that meets those requirements. If someone is having order of operations problems, imo the solution is not "go find (and grok) the internal system that you have issues with, then add a dependency once you've sorted that out". We need a more general way for users to "drop their system in the right place" and have it work.

I'm personally betting on "multiple labels" (as has been discussed elsewhere) as the solution to that problem. It lets api designers export generic "points" in the schedule where certain guarantees can be made. Until then, I think stages are still the solution to that problem (and maybe targeted public label exports as an interim less "blunt force" solution).

Maybe there's some concept of "private" systems that don't need labels because the user should never think about them, but that seems to run contrary to the bevy philosophy of open architecture, as I understand it.

Bevy definitely strives for "open architecture", but that is one of a number of constraints we are solving for. Other constraints include "simplicity", "ease of use", and "protection from foot guns". This also relates back to the "pub everything" point above. Making every internal system an "extension point" completely throws api design out the window.

@alec-deason
Copy link
Member Author

alec-deason commented Mar 4, 2021

That's fair. I've never considered system execution order as part of the api design, but you're right that it is. From that perspective, limiting the number of labels makes sense however it is so uncomfortable to not be able to hook into the system order when you need to that it seems to me over labeling is preferable to underlabeling. Maybe the multiple labels stuff achieves the same effect in a more controlled way.

@alice-i-cecile
Copy link
Member

Interesting, thanks for explaining that more elegantly @cart. I agree that multiple labels makes designing those sorts of APIs feasible.

Regardless, I think this discussion should happen in a more clearly visible place, and after we've attempted to re-order / add dependencies for all of the Bevy internal systems for 0.5. We'll have much more data there to make decisions with.

@cart
Copy link
Member

cart commented Mar 4, 2021

And to be clear: I'm not putting my foot down here. I just don't think we should open up this pandoras box until we've decided it is the right call.

Is anyone opposed to closing this pr (for now), only adding labels where needed for internal dependencies (and ideally leaving those labels private unless absolutely necessary), then formally designing the solution to the problems discussed above? If this pr was motivated by a user issue, can we record that requirement somewhere (if we haven't already) to help inform our decision making process?

@alice-i-cecile
Copy link
Member

Is anyone opposed to closing this pr (for now), only adding labels where needed for internal dependencies (and ideally leaving those labels private unless absolutely necessary), then formally designing the solution to the problems discussed above?

I'm in favor of this. Here's the original issue that prompted this PR on Discord.

Hello everybody, I have a random issue on master, not sure my code has wandered into undefined behavior or if it's a master issue. I have text that I try to decorate, so I set the text sections in a system, and I get the calculatedsize in another system running on PostUpdate, to be able to draw a fancy border around the text. Most of the time, this works great. Sometimes, it doesn't, meaning some sprites are not showing, not resized properly, not at the right z-index. I seem to go into my system OK. If I relaunch without changing anything it works. Would there be some timing issue me changing some Sprite Transform in PostUpdate? Is there a canonical way to do what I want?

@alec-deason
Copy link
Member Author

Yeah, works for me. I believe the only label the motivating problem requires is on text2d_system

@cart
Copy link
Member

cart commented Mar 4, 2021

Just linked to this from #1375. Lets continue the discussion there (and or some future Discussion)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants