-
Notifications
You must be signed in to change notification settings - Fork 128
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
Added text wrapping, consolidated text operations into new file #1542
base: master
Are you sure you want to change the base?
Conversation
I'm not sure if there is anything that needs editing in .openscad_docsgenrc for adding this new file, but I recommend checking it out. |
Typo "paragraph" in docs. Should indent function bodies. For consistency probably "text_array" instead of "textarray". (Note: before BOSL2 I never used underscores; that was Revar's requirement.) The text_array_boundingbox function doesn't return a bounding box, only a width and height. Maybe it should be renamed text_array_size? The array_text() module maybe should be rolled into text()? That is, if you pass a string it calls the current code and if you pass an array of strings or array of array of strings (?) it calls the new code. Also, this module doesn't have any attachable handling, which is bad. You should always call attachable() unless there's some very compelling reason you can't, in which case you need to manually support a bunch of stuff---so many things actually that half of them are missing from text() because I didn't realize I needed to add them there. But in particular, take a look at text(). Do you see all the stuff in there that sets $ variables and so on. If you don't call attachable you need to do all that stuff...and more. (Take a look at attachable and what it does when it creates the main child object and then the rest of the children. All that should be happening in text....and in array_text().) If array_text stays as a separate module I'm not sure about the name. Maybe it should be text_array() if it stays separate? But also, it should be grouped with text() as a rendering module in the docs. Maybe this is obvious, but we always need to think about laying our our code in the way that makes the most sense for a user reading the docs, NOT the way that makes the most sense for a programming reading the code! |
Hmmm. Relatively new no-arg version of attachable() may eliminate need to avoid attachable(), though some manual setting may still be necessary for $ variables. There's at least one line ending with ':' where the ':' could go to the next line. |
It looks like using attachable() with no args can indeed replace the stuff currently in text() and text3d() and provides added functionality. It looks to me like you have basically reimplemented the anchoring transform for a box instead of just invoking attachable() in array_text(), so it seems like you ought to be able to just invoke attachable with a size arg. |
The main reason, I think, why text() and text3d() don't do it that way is that they are designed to work in the stable OpenSCAD where we do not know the dimensions of the text. |
I note that the text() implementation can't have things attached to it, in fact the docs for text() state "You cannot attach children to text." All that's needed is to for it to be attachable with an anchor. That's what I was going for too. I tested it, and there's one example using the anchor. Merging array_text() into text() might save me the trouble of having to figure out attachments again. There may be a complication in that with multi-line text, How is a bounding box defined if not with width and height? Those are the dimensions of the box. The position of the box depends on the anchor. I could call it something besides bounding box, maybe I tend never to use underscores in variable names, just in module and function names. Not a problem to change, however. |
The reason you cannot attach children to text is that we don't know where the text is because we don't know it's size, so it is simply impossible to implement attachment. This is not true for text_array because you are assuming you have its dimensions, so what reason is there to not support attachment by just using attachable, which makes your code much simpler....and even more so when you consider what you'd need to add to make everything work without it. All the other attachable features need to work, so you need to make sure that tagging, highlighting, colors and attachment and so on all work. As I said, text() and text3d() are actually broken right now because highlighting and ghosting don't work. A bounding box is a size and position. As you say, this depends on the box position, hence it doesn't make sense to say you're producing a bounding box. Native OpenSCAD and hence also BOSL2 use "size" to refer to the dimensions of rectangles and cubes so the name should be text_array_size(). As I said, I don't use underscores at all...except in BOSL2 where we use them in all user-facing identifiers to separate all words. Merging array_text() into text() will not save you any effort because you're going to have to do it as a simple two-way conditional, where you invoke the old code if the old OpenSCAD is running and you have a single string, otherwise you invoke the new code. Keep in mind that attachable does a ton of stuff. If you want attach() to work right you need to make sure you have set all the right $ variables, for example, and set a geometry type. Getting all of that right is much harder than just invoking attachable(), and in this case no weird anchor overrides are required, so it's straight forward. The poor man's quasi-anchoring currently available in text() and text3d() is not something to aspire to. It's a workaround hack. This does raise a question, though, about anchoring consistency across versions. You can ban children like we do right now, but that seems a bit unfortunate. One would actually like to be able to do things like |
The only way to get the bounding box is to use textmetrics, which doesn't exist in 2021.01. Perhaps text() could be modified to use this and then the full attachment capabilities would be possible. Because the bounding box (which I shall rename to text_array_size) is a rectangle, and that's all we're concerned about for the purpose of attachments, how about for now I make the attachments the same as with square()? The more I think about it, the more I think array_text() should be separate from text(). There are minor differences that could lead to confusion if mashed together into the same module:
P.S. Instead of repurposing |
I think you're right that we should not wrap the new functionality into text(). I was losing sight of the fact that text() is a native openscad command. Your command should be a complete replacement for text and I'm not sure what it should be called, but I feel like it shouldn't have "array" in the name. It's just the new better command for doing text, like we have rect as an alternative to square and cuboid as an alternative to cube. There should probably also be a 3d version like text3d. I don't understand what the line_spacing parameter does. There should be a correct line spacing, which should be the default. The "spacing" parameter is also probably something questionable that perhaps isn't needed. Adding space between letters in a font is typographically bad generally speaking. The font was designed with the correct spacing. But the spacing parameter adds a multiplier which creates a ridiculous look. There's no reason you'd ever want to use it. If you actually wanted to screw up the font by spacing letters apart I think you'd want to add a fixed amount between each letter, not a proportional amount. It might also be appropriate for a BOSL2 font replacement module to fix the 0.72 factor bug in the font sizes. I'm not sure about that, though. |
The latest commit has some It works fine for snapshot OpenSCAD, but for 2021.01 it has weird behavior with parameters changing values. Try this test in a snapshot and in 2021.01.
The |
The default is I frequently make use of the |
Line spacing (also known as line height) is indeed commonly used and quite important for having the ability to control it effectively.
As an application and web developer with a UX background, I would disagree. There are valid use cases - for example, increasing letter spacing for buttons or headlines to enhance visibility and clarity. |
@amatulic For what it's worth, I’d love to see a version of
|
@Jasonkoolman - everything is already finished with respect to text justification, centering, line wrapping etc. The problem now is that we're considering making all of this into a wholesale replacement for BOSL2's existing However, when I made this PR I never intended to replace |
Thanks for the clarification! I’m not sure I fully understand the complexity of anchoring. In my mind, I’m imagining an extruded 2D shape with a rectangular bounding box. Are you thinking about adding anchors to the individual ‘faces’ of characters/glyphs, something of that nature? |
@Jasonkoolman - the anchors and attachment points would be at the center, corners, and edges of the text bounding box, as well as the left, center, and right ends of the text baseline. |
Re-tested examples, did some minor grammar fixing in comments.