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

Entity lighting fix #7626

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@hecktest
Copy link

hecktest commented Aug 7, 2018

This fixes the bug where entities would turn black if they had the origin at the feet, and were out of sunlight.
To make the fix more robust, a light_anchor property was created, replacing the hardcoded offset. The offset is applied in the entity's local space.
In order to easily transform said point to the entity's space, a typedef and a helper function for getting the scene node's matrix were added, since I couldn't find anything like that already in.
The default offset is +0.5 Y, which should work for almost any entity.

@SmallJoker
Copy link
Member

SmallJoker left a comment

Some style corrections. Is this WIP? Reading the properties from Lua and the docs do not exist yet.

Show resolved Hide resolved src/content_cao.cpp Outdated
Show resolved Hide resolved src/content_cao.cpp Outdated
Show resolved Hide resolved src/content_cao.cpp Outdated
Show resolved Hide resolved src/object_properties.cpp Outdated
Show resolved Hide resolved src/object_properties.h Outdated
@hecktest

This comment has been minimized.

Copy link
Author

hecktest commented Aug 7, 2018

Fixed everything except the default value; an offset of 0.5 Y is very safe for almost any object, and for the few that break, one can just override it.
Having a different default for the player and for the objects would add inconsistency for no good reason, and the old default of 0 breaks any entity modeled by someone who assumes 0 is the ground ( that is, anyone sane )

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Aug 7, 2018

I support the basic concept and like that this becomes adjustable, but i prefer that we keep it simpler, faster and how it is now by not transforming the offset into the entity space. There's not much need as light sampling position is almost always just a certain distance above the entity centre. There's very little benefit in rotating the offset with the entity as vertically in-line with the centre is usually the optimum position.
We could always add the transform later if it becomes essential for some reason (for example if we have entity pitch and roll, which might happen).
Collisionbox and selectionbox don't rotate with the entity, so it seems a little out of place with everything else.
I'm not disapproving this though, and maybe someone can convince me.

@paramat paramat added Bugfix and removed Documentation needed labels Aug 7, 2018

@hecktest

This comment has been minimized.

Copy link
Author

hecktest commented Aug 7, 2018

A one-dimensional Y-offset works for the most basic case, but it won't be enough if that rotation PR makes it in (and it should), it's not enough when you start attaching entities (a common case where lighting could break), and it won't be enough if you ever throw VAOs into the mix. This is a proper, future proof solution.
There are legitimate cases where you might want an "odd" offset for the light anchor, and since light is a visual thing, it makes sense to rotate it with the mesh. Multiplying one point by an existing matrix is a trivial operation, avoiding it for performance is not worth it.

@lhofhansl

This comment has been minimized.

Copy link
Contributor

lhofhansl commented Aug 8, 2018

This is a good idea. I can't judge the merits of the complexity vs. a simpler solution, though.

I assume there are no performance concerns since this is done fairly infrequently...?

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Aug 13, 2018

Yes there may well be good reasons to have XYZ offsets and rotate it with the object. However we can't consider things that might or might not get merged in the future, even though that pitch/roll rotation PR is quite likely to be merged.
VAOs are certainly not an argument as they are distant and uncertain.

But having written that, it may be justified for other reasons, do you have any specific common usecases? What do you mean by 'it's not enough when you start attaching entities (a common case where lighting could break)'?

Rotating with the object is not always correct. Many modders have the object centre as the zero point, and any object that uses 'visual = "wielditem" or "cube"' is this way too. Such an object will often need it's light anchor at a fixed position above or below the object centre however it is rotated.
Of course if the object zero point is at it's base, with the pitch/roll PR merged the light anchor then does need to rotate with the object.

So rotating the light anchor with the object needs to be optional, please could you do this? Most objects (dropped items, falling nodes) will not need the light anchor rotating with the object so this will avoid lots of unnecessary processing.

The performance impact will probably be small, but is still every step for every object, which can be a high rate of usage. Rotating a vector in 3D is quite a lot of calculation. I don't mean to imply it will be large. However all these little decisions we make, each with little performance effects, add up over time.

Rotating the light anchor with the object can always be added in a follow-up commit if object pitch/roll rotation is added. It might be best then to delay this PR a little until we process the object pitch/roll PR, which is likely to be soon.

I disapprove of this PR until rotation with object is made optional.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Aug 13, 2018

Maybe if the X and Z offset values are 'nil', just the Y offset is used and does not rotate with the object? Then the option is encoded into the offset value to avoid adding another object property for the option.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Aug 13, 2018

Most entities (dropped items, falling nodes, and most mod entities) will have no light anchor speciifed and will fallback to the default, for these the light anchor certainly should not rotate with the object.

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Aug 29, 2018

Rotating with the object is not always correct. Many modders have the object centre as the zero point, and any object that uses 'visual = "wielditem" or "cube"' is this way too. Such an object will often need it's light anchor at a fixed position above or below the object centre however it is rotated.

They can proceed using the center for the light position - nothing wrong with that. If you start rotating the objects with pitch & roll, then it makes sense to actually get the light where the model is.

Maybe if the X and Z offset values are 'nil', just the Y offset is used and does not rotate with the object?

That sounds like an ugly hack, and is totally counter-intuitive. Handle the vector the same in all cases is the best option here.

The performance impact will probably be small, but is still every step for every object, which can be a high rate of usage. Rotating a vector in 3D is quite a lot of calculation.

rotateVect: Vector initialization, 9 multiplications and 6 additions
get*Transformation: it is actually a bit heavier, because it uses a few trigonometric functions (setRotationRadians) and a bunch of variable assignments and some multiplications
These functions are called when the entity position updates - once per client env step.

Suggestion: Skip the transform part when light_anchor has a zero length. That's the only change that I'd like to see in this PR before it gets merged.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Aug 29, 2018

Handle the vector the same in all cases is the best option here.

Yes my random suggestion is messy, we can do better.

Suggestion: Skip the transform part when light_anchor has a zero length.

And when it is not specified.
This is better, although this means the only way to avoid the transform is to set the light sample point to object position. This means we cannot then set the light sample position with a simple non-transformed y-offset as players currently use. Almost entities do not need the transform but could benefit from a simple non-transformed y-offset. I think we should make this possible somehow, maybe using an additional parameter.
If this PR just added a non-transformed y-offset then we would have that useful ability.

What i request is the options:

  • Rotational transforming of light anchor.
  • A non-transformed y-offset of light sample position from entity position (default and set to zero offset).
    This option set as the default obviously, to be automatically applied to the large majortiy of entities (falling nodes, dropped items, simple entities) that do not need rotational light anchor transform, and to be applied to entities in already written mods that don't yet specify what behaviour they want.
    Currently this PR applies the transform to everything when the large majority of entities don't use it or need it, which is a lot of unnecessary processing.

Aside from all this, the pitch and roll PR has not been merged yet so this PR cannot be merged with an unnecessary 3D rotational transform, currently only rotation in the XZ plane is needed.

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Aug 30, 2018

Suggestion: Skip the transform part when light_anchor has a zero length.

And when it is not specified.

The light_anchor default value should of course be at (0,0,0) so it's in the center of the default 1x1x1m collisionbox.

This means we cannot then set the light sample position with a simple non-transformed y-offset as players currently use.

Sure we can, just use (0, 1, 0). Yaw changes alone won't have an effect on that Y value.

(falling nodes, dropped items, simple entities)

For the first two you don't even need a light anchor because the zero offset is in the center, which works very well.

Aside from all this, the pitch and roll PR has not been merged yet so this PR cannot be merged with an unnecessary 3D rotational transform, currently only rotation in the XZ plane is needed.

Sorry? Check out what attachments do; they let you rotate entities already.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Aug 31, 2018

Sure we can, just use (0, 1, 0)

That would be transformed, i wrote "we cannot then set the light sample position with a simple non-transformed y-offset". The ability to use a y-offset light sample point without the transform would be good to have, and we would have this ability if this PR didn't bother with a transform.

Check out what attachments do; they let you rotate entities already.

Ok, i forgot :)
///////////////////

The default offset is +0.5 Y, which should work for almost any entity.

It won't, what default vector to choose is a problem.
In current master, players have the sample point at 0.5 nodes above 'position' (feet level). Non-players have the sample point set to 'position'.
In the PR both players and non-players both use the light anchor vector added to position. So whatever default vector you choose, for mods that do not yet define an anchor, behaviour has been changed from before and in ways that causes bugs:

If the default is (0,0,0) then the sample point will be at player feet position so the player will probably go dark sometimes as it is on a node border.
If the default is (0,0.5,0) as in the PR, then non-player objects will have the sample point 0.5 nodes above the object centre. Some objects are very small so then the sample point will be some distance above the object, causing the object to go dark when near a ceiling.

I have to correct my earlier comment about 'adding another parameter', adding 2 new object properties is best avoided. I'm sure we can somehow encode everything into a single vector.

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Aug 31, 2018

If the default is (0,0,0) then the sample point will be at player feet position so the player will probably go dark sometimes as it is on a node border.

That's why player_api has to be extended afterwards to set this property on new players.

@hecktest

This comment has been minimized.

Copy link
Author

hecktest commented Sep 25, 2018

The defaults are now (0,0,0) for entities, and (0,0.5,0) for players. Good to go?

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Sep 25, 2018

No, see comments about avoiding the transform when not needed, and optionality. Still much to consider.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Sep 25, 2018

I'm not convinced transform is needed.
A model that is pitched and rolled will look weird unless it rotates around it's centre of mass. So the model zero point will be at the model's centre of mass. The optimum light anchor position is vertically in-line with that point, maybe a little above or below, and doesn't need transforming.

EDIT: I may be wrong, staying neutral.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Sep 25, 2018

Thanks, no objections from me now.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Sep 25, 2018

Please document in lua_api.txt the varying defaults for players and objects.

Show resolved Hide resolved doc/lua_api.txt Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment