Bug ID: JDK-8102591 Node Orienation - Canvas RTL orientation has not been implemented

JDK 8
8Fixed

I'm good with the approach, but I don't understand the part that uses the boolean for the layout in the NG layer - someone with a background in the text will have to review that.
09-10-2013
Calling: buf.putBoolean(theCanvas.getEffectiveNodeOrientation() == NodeOrientation.RIGHT_TO_LEFT); as part of fillText() barely touch performance, getEffectiveNodeOrientation() is cached in node, see: public final NodeOrientation getEffectiveNodeOrientation() { return (getEffectiveOrientation(resolvedNodeOrientation) == EFFECTIVE_ORIENTATION_LTR) ? NodeOrientation.LEFT_TO_RIGHT : NodeOrientation.RIGHT_TO_LEFT; } No performance penalty going with option (2)
09-10-2013
Felipe please investigate (2) and let us know the result. The code from 10/4 is not wrong. It's just that any time application code draw a string in a canvas, it must listen for effective orientation change and draw the canvas again. It is strange that API in node is affecting drawing in Canvas (hence RT-33436).
09-10-2013
#2 seems the cleanest from the surprise of the developer point of view. They may be aware of the buffer, but the presentation is mostly "draw this then draw that and it appears on the screen" which appears to be a synchronous and essentially immediate response, so an immediate reaction to a change in node orientation makes sense. WRT the example code from 10/4, I don't see it as "technically wrong", just that it is written to be intentionally divergent about rendering direction. We don't (and won't ever) promise to reorient strings when the orientation changes from pulse to pulse, so I see it as just an extension of that same lack of reorientation to changes that happen within a pulse. The programmer can predict what will happen if they reverse it on every pulse, and they can also predict what will happen if they reverse it every few lines of rendering. I, too, am interested in the performance issues. Perhaps Canvas nodes tend to have fewer parents than most nodes?
09-10-2013
Hard to tell Richard (performance impact), it depends how many draw text commands are issue and how many parents with orientation=inherit the canvas has. If you think that can be a problem we could copy the effective orientation of canvas to the GC (at creation) and listen for change events from the effectiveNodeOrientationProperty to keep it up to date.
09-10-2013
I think (2) is the right way to go. I liked (3) until I found out that GraphicsContext already had a reference to the Canvas from which it came, and that a single GraphicsContext was held by that Canvas. Since there is already a tight association, (2) makes the most sense. However, we need to do the due diligence to investigate the performance overhead of calling getEffectiveNodeOrientation a billion times (well, at least a few hundred thousand times :-)).
09-10-2013
I think (2), and we way the example of Oct-4 is technically wrong, no ?
09-10-2013
To summarize, I see only 3 ways: 1) Last orientation wins (last value at time of sync, asynchronous) 2) Draw each group of graphics context commands using the current orientation (current value, synchronous) 3) Cache the orientation in the GraphicsContext (value when created, might required new GC API to set/get it, see RT-33436) Each of these must behave in a reasonable manner with respect to the example code in this JIRA from Oct, 4 2013 01:35 PM.
09-10-2013
Hi Jim, it means that the effective orientation in canvas gets copied to the GC when getGraphicsContext() is callled. Then in GraphicsContext#writeText() this copy is used instead of theCanvas#getEffectiveNodeOrientation(). It seems that the original patch is the better option (the one that puts theCanvas#getEffectiveNodeOrientation() in the command buffer for every writeText operation) ?
09-10-2013
What does it mean to "cache the orientation in the GraphicsContext"? BTW, do you realize that there is only ever one single GC and no matter how many times you call getGC() you get the same exact one - containing all of the state that the last guy left in it...? (Clarify - one singleton permanent GraphicsContext per Canvas. Obviously 2 different Canvas objects have separate state)
08-10-2013
Note that the current behavior of width/height was noted as a bug in another context because of this discussion. When implementing "full clears reset the buffer" I noted that I am using values that aren't synchronized to implement that (and the buffer may never experience the indicated resize). Depending on how we go here, I may have to either abandon the reset work or start encoding the size changes into the buffer. So, yes, that is different than what we are discussing here, and orientation should have the same synchronization granularity as dimensions, but I am planning to make that sizing code compliant with our global policy that we are hashing out right now. One gotcha with the sizes is that they are set one at a time, but that isn't a big issue because the pixel retention behavior is independent in X and Y and we can always optimize the case of back-to-back "SETW/SETH" commands and do them both together...
08-10-2013
Sigh. This is quite complicated. Once upon a time, we were considering a third way: 3) Cache the orientation in the GraphicsContext Then, the code from the Oct 4th (assuming that the node was originally LTR), would draw everything RTL and the strings would be "inside out". This would happen because changing the orientation for the Node would have no effect on the GraphicsContext. It would happily draw LTR (including strings), then the final setNodeOrientation() would leave the Node RTL and the content would flip. The problem with this idea is that because the GraphicsContext is cached for the Canvas and the programmer can draw at any time, there is no place to update the cached context when the Node orientation changes. If we forced the programmer to do getGraphicsContext() to get something he could draw with, then we could do the update there. So, we could implement 3) and document that programmers need to call getGraphicsConext(). Speaking to Richard, we are wondering about implementing "2) Draw each group of graphics context commands using the current orientation". This would require that we get the effective orientation for each draw command. The advantage of this would be that no new API would be needed on Canvas to allow smart programmers to force drawing either to the left or the right (like the code in the comment from Oct 4th in this JIRA is trying to do). We need to investigate how expense getting the effective orientation would be.
08-10-2013
Ok, I spoke to Richard and he has convinced me that "last one wins" is wrong. It is correct to draw the string "inside out" and it is the responsibility of the programmer to draw when orientation changes and fix things. Further, we believe that we need LTR/RTL API for graphics context and we have worked out what that means. This will require a document or long meeting.
08-10-2013
Set orientation in the pulse like width and height.
08-10-2013
Personally I would like nodeOrientation to be handle in similar fashion as width and height. And correct me here if I'm wrong but I believe width and height are "sealed" in the pulse. And so should nodeOrientation. Furthermore, as width and height, the canvas has to be redraw when nodeOrientation changes. That is case either way. (I think that is the most important decision). What is our decision here ? I don't think this a big decision, but I think the like the last patch better (the patch the set the orientation in the pulse instead of in each draw call).
08-10-2013
It seems that the proposed behaviour allows the programmer to render strings that are drawn "inside out" which is something I think we should not do. I also understand that "last wins" is asynchronous and is conceptually inconsistent but I'm thinking that in practice it won't matter and that application code will see the "right thing" happening. I am not fanatical about "last wins" and realize that true asynchronous behaviour would require us to go back in time (something we can't do). Fully synchronous behaviour seems to be consistent and I could image example code where programmer ensured that a header and footer in a canvas was always drawn LRT and the content could be either. They would specify LTR, draw the header, specify INHERIT, draw the content, then specify LTR and draw the footer. Fully synchronous behaviour seems odd in that some API is on canvas and some is on node. I could imagine an API on canvas would make more sense but I remember Microsoft has such a thing on their HDC graphics context and this caused problems when the HWND had a different orientation. So, the problem remains. Programmers can write code that mixes orientation and graphics context draw commands. What should happen?
07-10-2013
Steve, I'm cool with Jim's proposed behavior as well. is that okay with you ?
05-10-2013
Here is another way to explain the point I was making above. The question is whether the orientation attribute on node is synchronous with respect to rendering commands. If you could change it and all strings that were ever written to the canvas, even if they were written many pulses ago, would switch direction, then it would be completely asynchronous. If you could change it and all strings that you render after the call that changes it are affected up until the next time you change it, then it would be completely synchronous. If you change it and it affects strings in the past and future that are part of the same pulse - and the developer has no concept of when the pulses happen because many of the operations they do are in event handlers and we have no indication of "this event handler was run on pulse #43559 and that event handler was run during pulse #43578" - then it is neither synchronous or asynchronous, it now somewhat unpredictably affects strings being rendered depending on forces outside of the developer's control (i.e. the ordering of event handling wrt synchronization and rendering sequences)...
04-10-2013
I'm aware of that test case and I know what it does. I'm actually fine with different strings being in different orientations because we already have that problem if you set it one way, then on the next pulse set it another way. It is, after all, what the developer wrote. Since we will never have a case where the strings are all guaranteed to be in the correct orientation always no matter when and where and how often you set the orientation, basing our entire decision on solving one piece of that issue - and a piece that is fairly obvious to the developer since the code all appears in the handling of a single pulse - is not very interesting. Also, consider that this is all a state machine already. When you fillrect and drawstring, we don't randomly decide to reverse those - they are executed in a particular order. Having some API related to Canvas that are state-ful and well-ordered and other parts that are asynchronous is inconsistent. There are pros and cons to either technique and I don't find that test case to be a singularly compelling motive...
04-10-2013
> I don't believe we ever get to updatePeer() if the screen is locked this is a bit surprising... i thought it was a plain java timer during the sync.. using the current values fails for the case - set size 200, 200 - draw a lot - fillRect white 100, 100 - draw a bit - set size 100, 100 I guess that case is not common enough for us to worry about it.
04-10-2013
Felipe - I don't believe we ever get to updatePeer() if the screen is locked. Also, the tests are dependent on the current attributes. We are already tracking those attributes in 2 places (in the GC object and in the NGCanvas object) and we'd have to develop a 3rd complete tracking of the state in order to evaluate "is this fill going to cover the canvas?". Since we are already tracking the attributes in GC, the tests are easy to do there. Steve - what is wrong with "the current value wins"? One problem with "last value wins" is that executing "set(LTR); drawString(...)" is an indeterminate sequence even if it is 2 adjacent lines of code written by the same developer. What advantage does "last value wins" have?
04-10-2013
they way I thought we would shrink the buffer in Canvas#impl_updatePeer() That happens regardless if the app is minimized or not, right ?
04-10-2013
Back to the node orientation discussion, I don't see a better way than "last wins" (actually, "last value at time of sync"). Let's implement that one for node orientation.
04-10-2013
No, that doesn't really help. The (main?) problem is that people are animating their hearts out while the window is not being sync'd (maybe because the app is minimized) leading to a crash. If we clear the buffer as we go, then people who are clearing the buffer for each frame of animation will never crash. The poor guy just updating some corner of the thing every frame of animation will still blow up though.
04-10-2013
since the actual rendering will happen some time later in NG thread not sure if using the current width/height is always the best answer. wouldn't it make sense to fill the buffer during rendering as we currently do. Then, as part of the sync, traverse the buffer (from the end to the start) looking for a fillRect if fillWidth>=width&&fillHeight>=height&&color.isSolid and clamp the buffer at that point ?
04-10-2013
The current code bases it on the width/height at the time the render call is made, whether or not that data has been synchronized yet.
04-10-2013
"That means that we can never detect any "full screen clear operations" and reset our buffers, which means we have no way (currently) to control animations that don't stop when a screen lock occurs." Would you remember the width/height of the last sync, the clear the buffer when fillRect covers that area ?
04-10-2013
Here is an interesting ramification of "last orientation wins". If that is the way we go here, then we set a precedent that "last value of a Node attribute is what affects rendering". This means that "last size of the canvas affects rendering to that canvas". That means that we can never detect any "full screen clear operations" and reset our buffers, which means we have no way (currently) to control animations that don't stop when a screen lock occurs. I am in the middle of adding that capability in another webrev, but I need to query the size of the canvas to determine if a render/clear operation is "full screen".
04-10-2013
RT26137_1.java contains "bogus code" that an application programmer might write. We need to do the "right thing" for this code (currently "Last orientation wins").
04-10-2013
Thanks. My feeling is that it should never be possible for the user to have strings draw strings "inside out" unless they (for example) draw the whole canvas LTR, change to RTL and then never draw again. This would be consistent with what happens with an image that contained rasterized text. Essentially, it's a programming error. The programmer can write code as follows in a method: GraphicsContext gc = canvas.getGraphicsContext2D(); gc.setFill(Color.YELLOW); gc.fillRect(0, 0, width, height); gc.setFont(new Font(20)); gc.beginPath(); gc.moveTo(0, 0); gc.lineTo(80, 80); gc.stroke(); canvas.setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT); gc.setTextAlign(TextAlignment.LEFT); gc.strokeText("Text.", 4, 60); gc.setFill(Color.BLACK); gc.fillText("line1111\nline222\nline", 10, 100); gc.setTextAlign(TextAlignment.CENTER); gc.fillText("line1111\nline222\nline", 80, 200); gc.setTextAlign(TextAlignment.RIGHT); gc.fillText("line1111\nline222\nline", 190, 300); canvas.setNodeOrientation(NodeOrientation.LEFT_TO_RIGHT); gc.setTextAlign(TextAlignment.LEFT); gc.fillText("small make to fit in 50px", 10, 400, 50); gc.setTextAlign(TextAlignment.CENTER); gc.fillText("small make to fit in 50px", 40, 450, 50); gc.setTextAlign(TextAlignment.RIGHT); gc.fillText("small make to fit in 50px", 80, 500, 50); canvas.setNodeOrientation(NodeOrientation.RIGHT_TO_LEFT); gc.setFill(Color.RED); gc.fillOval(4, 60, 4, 4); gc.fillOval(10, 100, 4, 4); gc.fillOval(80, 200, 4, 4); gc.fillOval(190, 300, 4, 4); gc.fillOval(10, 400, 4, 4); gc.fillOval(40, 450, 4, 4); gc.fillOval(80, 500, 4, 4); We need to decide what this will do. If our rule is that we should never draw strings "inside out", there are 2 choices: 1) Last orientation wins 2) Draw each group of graphics context commands using the current orientation I vote for 1) Last orientation wins. In future we could provide API for graphics context that controls LTR/RTL dynamically so that the programmer could force parts of the canvas to always be a certain orientation. This would be canvas API, not Node API.
04-10-2013
Steve, I uploaded a new version of the testcase, run it and hit 'enter' key to switch the orientation for the first canvas.
03-10-2013
Right, I'm just not sure what you meant by this sentence: "changing orientation while drawing will change the orientation as if the API was drawing directly" changing the orientation during drawing is not supported, and it would cause one group of the string to be backwards... The decision here really is: a) Require NodeOrientation to be set before the drawing starts (current implementation) b) Let it be set after the drawing but before the pulse.
03-10-2013
Ok, the behaviour is what I expected (ie. changing orientation while drawing will change the orientation as if the API was drawing directly). Canvas defaults to LTR for the same reason that ImageView defaults to LTR. If the application is smart and knows that what it is going to draw makes sense mirrored, then it sets the orientation to INHERIT (or RTL) and then draws. Just like an ImageView that is displaying an image that contains text, it will draw badly. In order to stop this happening, applications will have to listen for changes to effective orientation and draw again. Does this make sense and can we code an example that shows this?
02-10-2013
Another (minor) downside is that if node orientation must be constant during the command buffer execution then packing it in each fillText/strokeText command is not optimal. Should I change the code to grab the orientation during the synch ? What is policy for width and height ? Do they need to be set before the drawing to canvas starts ?
02-10-2013
A downside to the "grab the current orientation at the time that a drawString() method is called" approach is that this is the first time we have anything outside of the context of the attributes in the GraphicsContext affecting rendering. Right now all data for all rendering is entirely encapsulated in the GraphicsContext object. No information from outside that object is used while rendering. The current proposed fix would mean that GraphicsContext is no longer independent in its behavior.
02-10-2013
I should add that when I said "you also need to factor in the synchs" - Felipe's current fix grabs the orientation at the time of the drawString call on the fly, so it is not affected by when the synchronizations happen. But, some of the alternate proposals that involve grabbing the node orientation during the synch process and handing it down to the NG layer at that time would mean that the direction of the strings is snapshotted at synch time and so the sequences above then would depend on whether all of your strings are rendered during pulses where you left the orientation in a compatible setting right before a synch occured.
02-10-2013
Hi Steve, You also need to factor in when we synch on the pulses. Note that only strings are affected. Everything else is just rendered normally and if the texture for the canvas is reversed when copied to the screen then it doesn't matter. You can flip non-string rendering back and forth all day and it looks as you expected. It's only text - drawString calls - that have to be specially processed to flip them in place if you are rendering "backwards". Right now, every time you call "drawString" on a GC, Felipe's current fix will look at the node and tag that particular drawString call with the current value of the orientation at the time that you call drawString. This happens all at the FX level, so the calls to modify the orientation of the FX node are what is snapshotted into the render stream even if we haven't done any synching.. If you are constantly changing it while drawing strings, it will be constantly flipping various strings and you will have some strings forwards and some strings backwards. In the future, you can change the value on the node after all of that rendering is done, but the strings are now pixels and so they are set in their orientation so flipping after rendering will cause any strings rendered while the node attribute was different to appear backwards. Thus: canvas.setOrientation(LTR); drawString("First string"); canvas.setOrientation(RTL); drawString("Second string"); executed in a single pulse will have both strings displaying opposite of each other. At any future point in time, if you set the node to LTR then "First string" will look normal and the other will be backwards. Conversely, at any future point in time if you set the node to RTL then "First string" will be backwards and the other will look correct. Does that help? The main idea is (if there are strings in your rendering) always draw after you set the node orientation, and redraw completely after any time you change it thereafter. If you aren't drawing strings then you don't ever have to worry about the order of operations (but should we say that, or would it confuse the issue further?)...
02-10-2013
Sorry, this isn't clear to me. Please be patient. What happens when you: ... draw to canvas ... ... change orientation ... ... draw some more to canvas ... ... change orientation ... ... etc. Are we saying that the last orientation will win or that some of the drawing will be RTL and some LTR?
02-10-2013
To add to what Felipe pointed out - I don't think there will ever be a way to have the text look right if they change the orientation after the fact unless we either render both ways all the time, or keep a record of all drawing ever done to the Canvas. If we ever implement the "render on demand callback" API that will let us treat the Canvas as dynamic, we could trigger a repaint event when the node orientation changes, but for now we are pretty much stuck with an orientation once we start rendering text. The question is what granularity we want to use for setting it during initialization: - Must set it before any rendering that would be affected (mainly Text). - Must set it before first pulse Not really implementable, so off the table: - Can set it any time and we will flip flop as necessary (once we render, everything becomes pixels) Consider that the flavor of the Canvas right now is similar to that of an image - it's just a "render-y" way to make an image that can them move around your scene...
02-10-2013
Basing it on the current state seems to be the most predictable and covers more use cases.
02-10-2013
Richard & Steve & Leif: I just had a quick chat with Jim about how NodeOrientation, which is a node property, affects the rendering performed by GraphicsContext [from canvas.getGraphicsContext2D()]. Right now I'm sending the effective node orientation as part of each fillText/strokeText command, that means the node orientation must be set before the drawing starts, and must remains constant till prism renders the command buffer. Just changing the node orientation after the command buffer is consumed would cause the text to read backwards. Thus, every time the node orientation is changed the canvas should be re-rendered. Does that sound okay ? Another option is to pass the effective orientation during update_peer when the command buffer is hand over to prism, which means the last node orientation set between canvas draw start and the following pulse wins. Changing node orientation without rendering the canvas would still mess up the text...
01-10-2013
Ok then. I agree that we cannot defer (for now).
30-09-2013
Apparently so. it works for paths, lines, rect, etc. (didn't try anything too fancy, effects etc)
30-09-2013
So everything draws properly except the strings?
30-09-2013
Yes, it draw the string mirrored. I'll try to get this for sprint 67 (time allowing).
30-09-2013
I believe we can defer. My thinking is that we would need to add new API to Canvas that is analogous to RTL for nodes. This would move the origin to the top right etc. What happens now when RTL is set on a Canvas? Do strings draw inside out?
30-09-2013
I would like to defer this to Van Ness but I'm not sure this is the type of behavior we would be able to change in a future release. Steve, this is a must fix for Lombard ?
30-09-2013
The default orientation for Canvas will be Inherit (default). I'm okay with that. To control image drawing I liked the "image drawing mode" state for GraphicsContext you suggested.
08-02-2013
Canvas is a kind of direct drawing API. I believe that we flip them by default. We could consider API to drawImage(). I had thought about an API to force general LTR drawing, but this would affect where the image (or draw operation) happened, not the content of the draw operation so it would not be useful. Rather than adding an argument to each flavour of drawImage() we could add only to the most complex version or we could have an "image drawing mode" state for GraphicsContext. Thoughts?
06-02-2013
Note that we will need special code to handle string rendering (not fun). Besides, what is our strategy to handle images ? Do we flip them around with the canvas or do we handle them the same way as text ?
29-01-2013
Jim should be able to provide pointers into the Canvas code where one might place the transformation code.
08-11-2012