Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]
On Tue, 18 Oct 2022 22:15:35 GMT, Douglas Held wrote: >> The JavaDoc for equals had a copy/paste error. I normalized the text based >> on the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting >> in the method signatures of equals(), hashCode() and toString().~~ >> >> For good measure, some kind of copy/paste detection should probably be added >> to the many automated checks. For the entire OpenJDK project. > > Douglas Held has updated the pull request incrementally with one additional > commit since the last revision: > > Update Point3D.java > > Per review modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 414: > 412: /** > 413: * Indicates whether some other object is "equal to" this one. > 414: * Two instances of Point3D are equal if the return values of their `Point3D` also needs `@code`. modules/javafx.graphics/src/main/java/javafx/geometry/Point3D.java line 418: > 416: * > 417: * @param obj the reference object with which to compare > 418: * @return true if this Point3D is the same as the obj argument; > false otherwise `Point3D`, `true`, `false` and `obj` need `@code`. - PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]
On Tue, 18 Oct 2022 11:32:04 GMT, Nir Lisker wrote:
>> Agreed, mentioning equality criteria sounds good to me too. How does this
>> look ?
>>
>>
>> /**
>> * Indicates whether some other object is "equal to" this one.
>> * Two instances of Point3D are equal if the return values of their
>> * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
>> *
>> * @param obj the reference object with which to compare
>> * @return true if this Point3D is the same as the obj argument; false
>> otherwise
>> */
>
> This is the current proposal by Douglas and align with `Object::equals` which
> it is overriding. I'm fine with this wording. Just need to add the `@code`
> tags.
I have added a commit to match the proposed block by arapte.
-
PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]
On Thu, 13 Oct 2022 14:16:23 GMT, Nir Lisker wrote: >> Douglas Held has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update Point3D.java >> >> Per review > > Changes requested by nlisker (Reviewer). Tagged @nlisker to validate the code tags are included appropriately. - PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v4]
> The JavaDoc for equals had a copy/paste error. I normalized the text based on > the JavaDoc for method java.awt.Point#equals. ~~I also changed formatting in > the method signatures of equals(), hashCode() and toString().~~ > > For good measure, some kind of copy/paste detection should probably be added > to the many automated checks. For the entire OpenJDK project. Douglas Held has updated the pull request incrementally with one additional commit since the last revision: Update Point3D.java Per review - Changes: - all: https://git.openjdk.org/jfx/pull/913/files - new: https://git.openjdk.org/jfx/pull/913/files/0c52ce57..eb08b561 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=913=03 - incr: https://webrevs.openjdk.org/?repo=jfx=913=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/913.diff Fetch: git fetch https://git.openjdk.org/jfx pull/913/head:pull/913 PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Tue, 18 Oct 2022 17:47:38 GMT, Michael Strauß wrote: >> I'd respectfully disagree: in a large application, there might be hundreds >> (a thousand?) Nodes, and only a handful (10-20?) Nodes with this property >> (2%), so it's a waste of RAM. One may think it's only 8Kb on a 64 bit >> machine, but it does add up quickly. Even the access pattern is basically >> 'get it once to establish the fluent pipeline', with no more calls to the >> Node accessor (unlike MiscProperties constituents, some of which get >> accessed rather frequently). > >> I'd respectfully disagree: in a large application, there might be hundreds >> (a thousand?) Nodes, and only a handful (10-20?) Nodes with this property >> (2%), so it's a waste of RAM. One may think it's only 8Kb on a 64 bit >> machine, but it does add up quickly. Even the access pattern is basically >> 'get it once to establish the fluent pipeline', with no more calls to the >> Node accessor (unlike MiscProperties constituents, some of which get >> accessed rather frequently). > > I understand your point, but I don't think that this is sufficient reason to > introduce a new way of storing property instances in an ad-hoc manner. If > memory consumption is a concern, I would rather analyze the issue in a > separate enhancement proposal, think it through in terms of API, and then > transition all relevant property instances to the new storage mechanism. Part > of that proposal should also be an evaluation of the trade-offs between > memory consumption and performance in terms of cache locality and lookup > speed. @mstr2 : `getProperties()` is a perfectly fine way of storing a property instance, especially considering the fact that this property will be called exactly once - during the fluent pipeline setup. Of course, it does not apply to other properties that are being called all the time. A separate ticket for optimizing property storage might be a good idea, I agree. - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Tue, 18 Oct 2022 18:06:49 GMT, Nir Lisker wrote: > `getProperties()` isn't the place for this. can you explain why? - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Tue, 18 Oct 2022 17:47:38 GMT, Michael Strauß wrote: >> I'd respectfully disagree: in a large application, there might be hundreds >> (a thousand?) Nodes, and only a handful (10-20?) Nodes with this property >> (2%), so it's a waste of RAM. One may think it's only 8Kb on a 64 bit >> machine, but it does add up quickly. Even the access pattern is basically >> 'get it once to establish the fluent pipeline', with no more calls to the >> Node accessor (unlike MiscProperties constituents, some of which get >> accessed rather frequently). > >> I'd respectfully disagree: in a large application, there might be hundreds >> (a thousand?) Nodes, and only a handful (10-20?) Nodes with this property >> (2%), so it's a waste of RAM. One may think it's only 8Kb on a 64 bit >> machine, but it does add up quickly. Even the access pattern is basically >> 'get it once to establish the fluent pipeline', with no more calls to the >> Node accessor (unlike MiscProperties constituents, some of which get >> accessed rather frequently). > > I understand your point, but I don't think that this is sufficient reason to > introduce a new way of storing property instances in an ad-hoc manner. If > memory consumption is a concern, I would rather analyze the issue in a > separate enhancement proposal, think it through in terms of API, and then > transition all relevant property instances to the new storage mechanism. Part > of that proposal should also be an evaluation of the trade-offs between > memory consumption and performance in terms of cache locality and lookup > speed. `getProperties()` isn't the place for this. After looking at the grouping of properties, there might be merit to reevaluate the distribution of properties into their helper classes in the case that a single use of a common property will create many other unused ones. As for RAM usage, Hotspot can compress pointers on a 64bit machine. I don't think that the change in memory here will be more significant than in other areas in `Node` that can be addressed. - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Tue, 18 Oct 2022 16:03:39 GMT, Andy Goryachev wrote: > I'd respectfully disagree: in a large application, there might be hundreds (a > thousand?) Nodes, and only a handful (10-20?) Nodes with this property (2%), > so it's a waste of RAM. One may think it's only 8Kb on a 64 bit machine, but > it does add up quickly. Even the access pattern is basically 'get it once to > establish the fluent pipeline', with no more calls to the Node accessor > (unlike MiscProperties constituents, some of which get accessed rather > frequently). I understand your point, but I don't think that this is sufficient reason to introduce a new way of storing property instances in an ad-hoc manner. If memory consumption is a concern, I would rather analyze the issue in a separate enhancement proposal, think it through in terms of API, and then transition all relevant property instances to the new storage mechanism. Part of that proposal should also be an evaluation of the trade-offs between memory consumption and performance in terms of cache locality and lookup speed. - PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Tue, 18 Oct 2022 12:54:22 GMT, Kevin Rushforth wrote: >> Thanks, I understand, what I find odd is that this would be the first >> property in `Node` to be stored this way. If this was so important, then >> why isn't this done for other properties? There seem to be sufficient >> candidates for this approach to "slim" down `Node` (infrequently used >> properties like `Subscene`, `id`, `style`, `blendMode`). >> >> There's even a `MiscProperties` objects which is described as "Misc Seldom >> Used Properties" that is created only when needed with these properties: >> >> private LazyBoundsProperty boundsInParent; >> private LazyBoundsProperty boundsInLocal; >> private BooleanProperty cache; >> private ObjectProperty cacheHint; >> private ObjectProperty clip; >> private ObjectProperty cursor; >> private ObjectProperty depthTest; >> private BooleanProperty disable; >> private ObjectProperty effect; >> private ObjectProperty inputMethodRequests; >> private BooleanProperty mouseTransparent; >> private DoubleProperty viewOrder; >> >> Let's see what others think before I change this. It does sound like a >> reasonable approach though. > > I don't have time to review this as I am busy with JavaOne, so this is just a > quick note to say that if there is sufficient benefit to add a `shown` > property to the core of JavaFX, then it should be added _as_ a property -- > possibly in MiscProperties if we think it is a seldom-used property. I don't > like the idea of using `getProperties` for this. I'd respectfully disagree: in a large application, there might be hundreds (a thousand?) Nodes, and only a handful (10-20?) Nodes with this property (2%), so it's a waste of RAM. One may think it's only 8Kb on a 64 bit machine, but it does add up quickly. Even the access pattern is basically 'get it once to establish the fluent pipeline', with no more calls to the Node accessor (unlike MiscProperties constituents, some of which get accessed rather frequently). - PR: https://git.openjdk.org/jfx/pull/830
Integrated: 8294722: FX: Update copyright year in docs, readme files to 2023
On Mon, 17 Oct 2022 09:36:55 GMT, Ambarish Rapte wrote: > Update Copyright year in these 3 doc files to 2023. This pull request has now been integrated. Changeset: f448e9ad Author:Ambarish Rapte URL: https://git.openjdk.org/jfx/commit/f448e9ad589caa01f24c345d1db37c933c8e0dc8 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod 8294722: FX: Update copyright year in docs, readme files to 2023 Reviewed-by: kcr - PR: https://git.openjdk.org/jfx/pull/917
Re: RFR: 8217853: Cleanup in the D3D native pipeline [v7]
On Thu, 1 Sep 2022 11:23:44 GMT, Nir Lisker wrote: >> Refactoring and renaming changes to some of the D3D pipeline files and a few >> changes on the Java side. These are various "leftovers" from previous issues >> that we didn't want to touch at the time in order to confine the scope of >> the changes. They will make future work easier. >> >> Since there are many small changes, I'm giving a full list here: >> >> **Java** >> >> * `NGShape3D.java` >> * Extracted methods to help with the cumbersome lighting loop: one method >> per light type + empty light (reset light) + default point light. This >> section of the code would benefit from the upcoming pattern matching on >> `switch`. >> * Normalized the direction here instead of in the native code. >> * Ambient light is now only set when it exists (and is not black). >> * `NGPointLight,java` - removed unneeded methods that were used by >> `NGShape3D` before the per-lighting methods were extracted (point light >> doesn't need spotlight-specific methods since they each have their own "add" >> method). >> * `NGSpotLight.java` - removed `@Override` annotations as a result of the >> above change. >> >> **Native C++** >> >> * Initialized the class members of `D3DLight`, `D3DMeshView` and >> `D3DPhongMaterial` in the header file instead of a more cumbersome >> initialization in the constructor (this is allowed since C++ 11). >> * `D3DLight` >> * Commented out unused methods. Were they supposed to be used at some >> point? >> * Renamed the `w` component to `lightOn` since it controls the number of >> lights for the special pixel shader variant and having it in the 4th >> position of the color array was confusing. >> * `D3DPhongShader.h` >> * Renamed some of the register constants for more clarity. >> * Moved the ambient light color constant from the vertex shader to the >> pixel shader (see the shader section below). I don't understand the >> calculation of the number of registers in the comment there: "8 ambient >> points + 2 coords = 10". There is 1 ambient light, what are the ambient >> points and coordinates? In `vsConstants` there is `gAmbinetData[10]`, but it >> is unused. >> * Reduced the number of assigned vertex register for the `VSR_LIGHTS` >> constant since it included both position and color, but color was unused >> there (it was used directly in the pixel shader), so now it's only the >> position. >> * `D3DMeshView.cc` >> * Unified the lighting loop that prepares the lights' 4-vetors that are >> passed to the shaders. >> * Removed the direction normalization as stated in the change for >> `NGShape3D.java`. >> * Reordered the shader constant assignment to be the same order as in >> `D3DPhongShader.h`. >> >> **Native shaders** >> * Renamed many of the variables to what I think are more descriptive names. >> This includes noting in which space they exist as some calculations are done >> in model space, some in world space, and we might need to do some in view >> space. For vectors, also noted if the vector is to or from (`eye` doesn't >> tell me if it's from or to the camera). >> * Commented out many unused functions. I don't know what they are for, so I >> didn't remove them. >> * `vsConstants` >> * Removed the light color from here since it's unused, only the position >> is. >> * Removed the ambient light color constant from here since it's unused, >> and added it to `psConstants` instead. >> * `vs2ps` >> * Removed the ambient color interpolation, which frees a register (no >> change in performance). >> * Simplified the structure (what is `LocalBumpOut` and why is it called >> `light` and contains `LocalBump`?). >> * `Mtl1PS` and `psMath` >> * Moved the shader variant constants (`#ifndef`) to `Mtl1PS` where they >> are used for better clarity. >> * Moved the lights loop to `Mtl1PS`. The calculation itself remains in >> `psMath`. >> >> No changes in performance were measured and the behavior stayed the same. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Addressed review comment @kevinrushforth If this fell through the cracks please add it to the list too :) It's a blocker for future work. - PR: https://git.openjdk.org/jfx/pull/789
Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
On Mon, 27 Apr 2020 11:43:28 GMT, John Hendrikx wrote: > This fixes a bug where the first call to unbind would clear the internal > invalidation listener used, resulting in subsequent unbind calls to be > no-ops, unless bind was called again first. > > I had to rewrite the parameterized test slightly as Parameterized will only > call the parameters method once, and my new test modifies the internal state > of the bindings used as parameters (by doing some unbind calls) which was > making other tests fail. Yes, it did fall through the cracks. I wasn't sure it was still relevant, but since it is, I'll put it on my queue. Perhaps either @arapte or @nlisker can be the second reviewer. - PR: https://git.openjdk.org/jfx/pull/198
Re: RFR: 8290040: Provide simplified deterministic way to manage listeners [v6]
On Tue, 18 Oct 2022 05:40:59 GMT, John Hendrikx wrote:
>> private static final Object KEY = new Object();
>>
>> public final ReadOnlyBooleanProperty shownProperty() {
>> Object x = getProperties().get(KEY);
>> if (x instanceof ReadOnlyBooleanProperty p) {
>> return p;
>> }
>> ReadOnlyBooleanProperty p = ... // create
>> getProperties().put(KEY, p);
>> return p;
>> }
>
> Thanks, I understand, what I find odd is that this would be the first
> property in `Node` to be stored this way. If this was so important, then why
> isn't this done for other properties? There seem to be sufficient candidates
> for this approach to "slim" down `Node` (infrequently used properties like
> `Subscene`, `id`, `style`, `blendMode`).
>
> There's even a `MiscProperties` objects which is described as "Misc Seldom
> Used Properties" that is created only when needed with these properties:
>
> private LazyBoundsProperty boundsInParent;
> private LazyBoundsProperty boundsInLocal;
> private BooleanProperty cache;
> private ObjectProperty cacheHint;
> private ObjectProperty clip;
> private ObjectProperty cursor;
> private ObjectProperty depthTest;
> private BooleanProperty disable;
> private ObjectProperty effect;
> private ObjectProperty inputMethodRequests;
> private BooleanProperty mouseTransparent;
> private DoubleProperty viewOrder;
>
> Let's see what others think before I change this. It does sound like a
> reasonable approach though.
I don't have time to review this as I am busy with JavaOne, so this is just a
quick note to say that if there is sufficient benefit to add a `shown` property
to the core of JavaFX, then it should be added _as_ a property -- possibly in
MiscProperties if we think it is a seldom-used property. I don't like the idea
of using `getProperties` for this.
-
PR: https://git.openjdk.org/jfx/pull/830
Re: RFR: JDK-8295236: Update JavaDoc in javafx.geometry.Point3D [v3]
On Tue, 18 Oct 2022 04:47:38 GMT, Ambarish Rapte wrote:
>> I think that we should explain what makes 2 objects equal. Even if we don't
>> explicitly name the methods used for comparison, we could say "2 points are
>> equals if their coordinates are equal".
>>
>> By the way, I have https://bugs.openjdk.org/browse/JDK-8226930 assigned to
>> go over some dubious equals/hashcode implementations, in case you want to
>> delegate the task.
>
> Agreed, mentioning equality criteria sounds good to me too. How does this
> look ?
>
>
> /**
> * Indicates whether some other object is "equal to" this one.
> * Two instances of Point3D are equal if the return values of their
> * {@code getX}, {@code getY}, and {@code getZ} methods are equal.
> *
> * @param obj the reference object with which to compare
> * @return true if this Point3D is the same as the obj argument; false
> otherwise
> */
This is the current proposal by Douglas and align with `Object::equals` which
it is overriding. I'm fine with this wording. Just need to add the `@code` tags.
-
PR: https://git.openjdk.org/jfx/pull/913
Re: RFR: 8243115: Spurious invalidations due to bug in IntegerBinding and other classes
On Tue, 28 Apr 2020 00:00:28 GMT, Kevin Rushforth wrote: >> I will review this too anyway. > >> I will review this too anyway. > > Thank you. That will be helpful. @kevinrushforth I was going through my open JDK tickets, and saw that this slipped through the cracks. This ticket would need another reviewer still, @arapte could you take a look? - PR: https://git.openjdk.org/jfx/pull/198
