Quaternion: refactoring and minor API improvements #2522
Conversation
|
What's the use case here? Quaternion already has getX() methods etc |
To maintain consistency API with the public variables of the classes |
|
Consistency isn't an end in and off itself if it makes things worse. Making these public means we can never change the way quaternion is internally represented |
|
I've always though it was a big mistake to make x,y,z,w fields public in the vector classes, in the first place. If anything we should figure out how to deprecate the public fields, IMO. |
|
Yes, guys, I know it's commonly discouraged to access public variables in a class. However, the context changes in graphics engines. Here's why: Pros (in a Java Graphics Engine Context)When a class like
|
|
If it really mattered I'm pretty sure the JVM would inline the getter anyway for performance reasons. But how often do callers really want to get access to the w parameter on a quaternion. My vote would be to not do this |
I'm far from an expert in these types of micro-optimizations. (and I do not want to be, if we are being honest) But from my understanding and a brief searching to refresh my memory, you are correct:
I nearly always lean towards totally ignoring these types of micro-optimizations because (even in cases where there is a small benefit) they will never actually make a game run faster. As I often mention, the bottleneck in games is always related to meshes/shaders and the GPU. And in cases where the CPU is the bottleneck, then it is always related to something major like too many ray casts, and focusing on micro-optimizations (such as a getter vs public fields) is extremely trivial and non impactful. I personally find this aspect of coding games to be liberating. Games are so big and bloated to begin with, so there's no reason to focus on these micro-optimizations. There certainly are industries where micro optimizations have an impact, but not games. I can sooner save memory or significantly speed up my game by staggering ray casts or reducing pixels in an image. But micro-optimizations will never have any real impact on a game for the end user, so i can freely ignore them and see next to no consequences (this is also why I've mentioned I'm likely not the best to review these types of PRs, but I'm still willing to give my opinions or merge them in cases where there is a proven benefit). |
|
Regarding the proposal to directly expose fields in Core math classes like All these classes are declared as Bringing The primary benefit of this adjustment is the ability to have direct variable access in high-computation areas such as I understand that strict adherence to getters/setters is generally considered good programming practice. However, in the context of |
I actually disagree with this, partly. The priority is most certainly not efficiency at all costs in a game engine. Engines already accept extremely high computational operations on the GPU that make intensive CPU operations look like nothing. CPU operations that would bog down other time-sensitive apps are barely a drop of water in an ocean when viewed in the context of a computationally expensive game engine. In a game engine, these minor optimizations that speed up the code by 1/10000 of a second are completely useless. I feel like I've repeated this many times, but I will say it again because it is very important in context of a game engine and when deciding what to spend your time optimizing: The bottle neck of a slow game is never going to be inefficient primitive code on the CPU unless the user is already doing something very wrong. So while it may theoretically be faster to make tiny optimizations to the codebase like this, the fact of the matter is that it will almost never matter in a game engine. Also I do not think consistency is that important either. Atleast not as a factor in and of itself. There are often good reasons for doing things inconsistently between classes that otherwise look very similar. Often for good or intentional reasons. And (to play devlis advocate) I could also raise the point that familiarity is indeed also an important part of keeping code consistent and maintainable. Every minor change like this reduces the level of familiarity for other jme devs who go back and look at a class they previously had experience using. Ther are a handful of jme devs out there who know certain classes or parts of the codebase like the back of their hand, and when we constantly make changes, this reduces that level of familiarity. So it is important that there is a very good and provable reason for absolutely every change that gets made to the engine's codebase. |
| * @throws IOException from the exporter | ||
| */ | ||
| @Override | ||
| public void write(JmeExporter e) throws IOException { | ||
| OutputCapsule cap = e.getCapsule(this); |
There was a problem hiding this comment.
I think that minor changes to variable names like this should be avoided when possible.
I would be just as content with the variable named "oc" or "o" or "outputCapsule" or even "outputCap" and for exception to be "e" or "exc" or "ex" or "exception" and I think that being indifferent to this is a beneficial skill for developers. My own code is full of a mixture of these, and it really depends how I felt that day when I was coding.
I actually strongly believe that it is good practice for a coder to not form a preference in these regards, as it leads to time wasted when things don't fit your preference. And in a room with 10 devs, they will all tell you they prefer it a different way of naming variables and lead to an unproductive debate. So I opt to accept all variable naming formats (as long as they are sensical or spell out the whole word or use acronyms appropriately)
So I'd suggest trying to minimize these types of changes where it doesn't offer much objective benefit, just to keep the review process as fast and focused as possible.
I don't agree that removing the
Please stop with the ai slop, just share a benchmark that proves that it is more efficient to use public fields vs relying on jvm inlining and you'll convince everybody. I gave it a try: public field public final class Public {
public float x;
}
public class Test1 {
public static void main(String[] args) {
Public p = new Public();
long t = System.currentTimeMillis();
for(int i=0;i<2_111_000_000;i++) {
p.x += Math.ceil(Math.random() * 1000);
if(p.x%2 == 0) {
p.x = 0.0f;
} else {
p.x = 1.0f;
}
}
System.out.println("Value of x: " + p.x);
System.out.println("Public) Time taken: " + (System.currentTimeMillis() - t));
}
}getter public class Getter {
private float x;
public float getX() {
return x;
}
public void setX(float x) {
this.x = x;
}
}
public class Test2 {
public static void main(String[] args) {
Getter p = new Getter();
long t = System.currentTimeMillis();
for(int i=0;i<2_111_000_000;i++) {
p.setX(p.getX() + (float)Math.ceil(( Math.random() * 1000)));
if(p.getX()%2 == 0) {
p.setX(0.0f);
} else {
p.setX(1.0f);
}
}
System.out.println("Value of x: " + p.getX());
System.out.println("(Getter) Time taken: " + (System.currentTimeMillis() - t));
}
}they run identically riccardo@riccardo-ws:~/PROJ/DEV/test1$ javac Test1.java
riccardo@riccardo-ws:~/PROJ/DEV/test1$ javac Test2.java
riccardo@riccardo-ws:~/PROJ/DEV/test1$ java Test1
Value of x: 1.0
(Public) Time taken: 45839
riccardo@riccardo-ws:~/PROJ/DEV/test1$ java Test2
Value of x: 1.0
(Getter) Time taken: 45590 |
|
To extend on what riccardobl is saying. AI is certainly a useful tool (I use it myself) but it needs to be guided for a useful purpose (and aggressively checked). You've produced a lot of changes in a lot of PRs very fast using AI. Some of them have been good, but a lot of them have felt like changes for changes sake and that means review and testing needs to be done for a change that didn't really need to happen in the first place |
|
Thanks for all your feedback, everyone. The widespread "thumbs up" emojis definitely caught my attention. My aim is to contribute positively, regardless of whether a specific PR is accepted. The project isn't mine to own, and if my suggestions don't align, I'm okay with that. We can always continue to exchange ideas and maintain our collaborative spirit. |
|
Yeah, i didn't mean to sound too harsh, I think you are doing a good job at passing the engine through ai. My feedback is just that you should avoid making ai argue for you, most LLMs are too agreeable, they will validate your assumptions even if they are wrong. Or, if you want to share an argument made by an ai that you agree with, please make it write a short summary of the points it is making, and add a disclaimer saying it is ai generated content and from which model it comes, so people can take it with a grain of salt. If there is a point of contention, IMO, let the code speak (a benchmark in this case). |
|
I discovered all the bugs related to cloning and serialization by writing tools and demos. Now, I pay close attention to these details because they almost always present a problem. I only use AI for the analysis and optimization of more complex classes that lack comments or Javadoc. However, I meticulously analyze and verify every code change. I never take anything for granted. Edit: |
|
You could still leave this open for the other parts that are okay (like the formatting, imports and copyright) if you'd like to still update things and submit those parts of this pr (or you can close it if that's your preference) |
ok, the |
capdevon
changed the title
write/read).toAxes()to use Java array conventions.