Skip to content

Conversation

alelievr
Copy link
Member

@alelievr alelievr commented Jul 27, 2021

Purpose of this PR

  • Fixed algorithm so that a distance of 0 doesn't screw the rotation
  • Fixed Light anchor fields not working in prefabs (except orbit, elevation, and yaw which are not serialized in this component)
  • Remove the dependency on the light component
  • Added an anchor position override transform component to have a physical target in the scene.
  • Fixed the anchor rotation being reset when the object is copy/pasted
  • Light anchor modifier tool now works correctly with rotation pivot mode

https://fogbugz.unity3d.com/f/cases/1345509/

Note that if the anchor position override is not null, the light anchor transform will be updated each time the target transform updates.


Testing status

tested that the light anchor distance works correctly + distance and Up direction handle correctly prefabs.

LightAnchor.mp4
LightAnchor2.mp4

image

Note that there is an issue with Light Anchor prefabs when the distance is reverted, the transform of the gameobject is not updated using the reverted distance value (I'm not sure if we can fix that looking at how the component works currently)

The light angle is now adjusted with the camera position instead of rotation to correct the perspective

LightAnchor4.mp4

https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/hd%252Ffix%252Flight-anchor/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk/8484994/job

@alelievr alelievr added the HDRP label Jul 27, 2021
@alelievr alelievr self-assigned this Jul 27, 2021
@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/.yamato%252F_abv.yml%2523all_project_ci_2021.2
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

@github-actions github-actions bot added the SRP label Jul 27, 2021
@alelievr alelievr requested a review from a team July 27, 2021 14:14
@alelievr alelievr marked this pull request as ready for review July 27, 2021 14:15
@alelievr alelievr requested a review from RSlysz July 28, 2021 10:50
@alelievr alelievr marked this pull request as draft July 28, 2021 13:04
@skhiat skhiat marked this pull request as ready for review August 10, 2021 14:55
@skhiat skhiat requested a review from a team August 10, 2021 14:56
@iM0ve
Copy link
Contributor

iM0ve commented Aug 30, 2021

Test status: Done

I have tested all the parameters using a simple scene where I used spheres for anchor points. Then switched to lights (area and spot) and tried to light Adam to mimic the main use case in real projects. No major issues found only this once ux concern, if its valid.

Concerns:

  1. The new "light look at" property is great, but the Pivot is used as the anchor point instead of Centre of the object. I assume in some cases this could be an issue? For instance in the video I wanted to focus the light on Adams head, but the Pivot position was at his feet so the light ended up looking towards the ground. At the end of the vid i show where is the centre vs pivot location

There is a workaround for this, you can create an empty game object and place it where you want the light to look at(for example as child of Adams head). But I dont know what is more common, to focus on Pivot or create these dummy objects to workaround the issue.

Screen.Recording.2021-08-30.at.15.56.55.mov

@pierre-unity
Copy link
Contributor

pierre-unity commented Aug 31, 2021

I would suggest we also add an new "Position offset" under "Anchor Position Override" (indented), for all cases where the pivot of the "Anchor Position Override" is not exactly where it should logically be.

For instance, sometimes that pivot of a character is at the heal of the character, but the light should rather orbit the head. So with a Y-offset of roughly 1.6m, the light will be orbiting the right bit of the character.

@pierre-unity
Copy link
Contributor

Another issue, related to very high distances: it creates a "pivot drift", which does not happen with a custom anchor position override btw.

2021-08-31_17-56-45.mp4

@pierre-unity
Copy link
Contributor

pierre-unity commented Aug 31, 2021

Another potential improvement:

Can we get the list of all the bones from a gameobject, and let the user pick a bone from this list?

Basically, if I pick an "Anchor Position Override" that has bones, we then display a drop down list of all bones attached to it and let the user pick a given bone transform.

See here how to get the list:
https://forum.unity.com/threads/solved-how-can-i-collect-bone-names-and-transforms.35810/#post-232041

@alelievr
Copy link
Member Author

alelievr commented Sep 1, 2021

So we'll move forward and merge the PR as it is now and do further improvement in a second PR (This PR is already too old :) )

@sebastienlagarde sebastienlagarde removed the request for review from a team September 1, 2021 10:11
@sebastienlagarde sebastienlagarde merged commit b30737d into master Sep 1, 2021
@sebastienlagarde sebastienlagarde deleted the hd/fix/light-anchor branch September 1, 2021 16:11
sebastienlagarde pushed a commit that referenced this pull request Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants