Skip to content

Conversation

@Markil3
Copy link
Contributor

@Markil3 Markil3 commented Feb 2, 2021

As @tlf30, @pspeed42 and I were working on #1443, it became apparent that the LightControl class wasn't entirely functional, particularly when it came to spot lights. This PR not only fixes that (including filling out the missing spotlight code in the lightToSpatial method), but also replaces some legacy code relating to directional lights from before version 3.
https://hub.jmonkeyengine.org/t/gltf-unlit-material-and-punctual-light-extensions/44132/29?u=markil3

Note that, when dealing with "world" coordinates when moving the spatial to the light, the rotations aren't always entirely accurate (the distance between the vectors can be as much as 4 units during testing). However, they appear close enough in debugging that I'm not concerned (Line 141 of TestLightControl2Spot).

…pot lights to better handle rotations.

The directional light was using outdated code from before 3.0 that didn't make sense in these days, to the point of confusion whenever the

Also, the spotlight code wasn't working properly, as can be demonstrated by the new TestLightControlSpot test.

Thanks @pspeed42 for the help with simplifying the quaternion.
…obal coordinates and spot lights.

The spot light wasn't implemented at all, and the directional light didn't work.
There should be far less (although one will inevitably pop up when the test first starts, for some reason.
@stephengold stephengold added this to the v3.4.0 milestone Feb 25, 2021
@Markil3 Markil3 requested a review from stephengold February 25, 2021 23:00
@MeFisto94
Copy link
Member

It is nit picking, but would you mind following https://app.codacy.com/gh/jMonkeyEngine/jmonkeyengine/pullRequest?prid=6891987?
Especially because some of those are examples and thus being over-correct style wise might not be bad.
Plus we improve a ficitional number :p

@Markil3
Copy link
Contributor Author

Markil3 commented Mar 3, 2021

Right, Codacity is now happy with the PR. It does have a duplication rating of +28, but that is likely due to the fact that the four tests I contributed are technically just copy/pastes of each other with minor changes.

@stephengold
Copy link
Member

I'm not convinced that lightToSpatial() does what it ought to. For one thing, it tries to rotate the spatial for a point light (which has no direction) and translate the spatial for a directional light (which has no location). What's the intention here?

@Markil3
Copy link
Contributor Author

Markil3 commented Mar 7, 2021

At lines 184 through 186, I just set the rotation and translation of the spatial based on vect1 (translation) and vect2 (direction). Since PointLight doesn't have a direction and DirectionalLight doesn't have a position, I just fill those fields with what the spatial already has (lines 168 and 171). This way, a PointLight won't change the existing rotation of a spatial and DirectionalLight won't change the existing position. The alternative would be to reset the rotation or translation of the spatial with a default, but I decided against it.

@stephengold
Copy link
Member

You fill those fields with the local translation and direction, relative to the parent. But that doesn't work if the parent happens to be translated and rotated. Wouldn't it be simpler and less error-prone to update only the translation OR rotation in those cases?

@Markil3
Copy link
Contributor Author

Markil3 commented Mar 7, 2021 via email

…n't have those options.

It is a little less error-prone.
@stephengold
Copy link
Member

stephengold commented Mar 18, 2021

I appreciate your cooperation and patience. A few more items:

  • Please add JMonkeyEngine copyright notices to each of the 4 added files.
  • Please update the years in the "LightControl.java" copyright notice to include 2021.
  • When the test apps are run, it's difficult to tell what's happening because (1) the light moves too fast and (2) the initial camera position is too close to the light.
  • The test apps could be made more readable by (1) deleting commented-out code and (2) breaking complicated vector-math expressions into multiple statements in the "make sure they are equal" tests
  • Both "TestLightControl2Spot" and "TestLightControl2Directional" contain the following useless code:
        /*
         * Offset the light node to check global stuff
         */
        Node axis = new Node();
        axis.setLocalTranslation(5, -3, 2.5F);
        axis.setLocalRotation(new Quaternion().fromAngles(FastMath.PI / -4F, FastMath.PI / 2F, 0));
        axis.attachChild(lightNode);

It seems to me that axis is never attached to the scene graph, and lightNode gets reparented to the root node immediately afterward.

@Markil3 Markil3 requested a review from stephengold March 22, 2021 21:25
Copy link
Member

@stephengold stephengold left a comment

Choose a reason for hiding this comment

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

Much improved.

@stephengold stephengold merged commit 344c491 into jMonkeyEngine:master Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants