Skip to content
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

Add C1 C2 prefabs, and camera featues #280

Merged
merged 25 commits into from
May 27, 2024
Merged

Conversation

lchojnack
Copy link
Contributor

@lchojnack lchojnack commented Mar 15, 2024

List of changes:

  • Add C1 and C2 camera prefabs
  • Change the thread group for RosImageShader
  • Add Camera features:
    • Image adjustments
      • Hue
      • Color Saturation
      • Contrast
      • Brightness
      • Sharpness
    • Exposure settings
      • Auto Exposure
      • ISO
      • Shutter Speed
      • Aperture
    • Lens Distortion Correction
    • Additional Camera Effects available in Unity
      • Bloom
      • Chromatic Aberration
      • Depth of Field
      • Motion Blur

@szylis szylis marked this pull request as ready for review April 3, 2024 10:54
@mackierx111
Copy link
Collaborator

@lchojnack

I have merged the main branch into the feature/c1_c2_cameras branch in my local environment and got an error. can you please merge the main branch and resolve the error?

image

@szylis
Copy link
Contributor

szylis commented May 10, 2024

@mackierx111 and @lchojnack

I've updated branch to main one and I've added missing rendering pipeline references to AWSIM assembly. The errors should be gone now.

Copy link
Collaborator

@mackierx111 mackierx111 left a comment

Choose a reason for hiding this comment

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

@lchojnack @szylis
Thanks for the implementation. Including a chart or checkerboard image for testing is a good idea.

I have some questions.

  • Is it correct that the main purpose is to reproduce C1 and C2 cameras using volume profiles and HDAddtionalCameraData component?
  • Am I correct that CameraSensorFeatureManager is a class that sets HDAddtionalCameraData and Volume/VolumeProfile from a script?
    • Is there a problem with setting HDAddtionalCameraData and Volume/VolumeProfile without CameraSensorFeatureManager?
  • What are your thoughts on integrating the CameraSensorFeatureManager and CameraSensor classes?
  • Can you please add documentation on the C1, C2 camera or this camera implementation?

@mackierx111
Copy link
Collaborator

MEMO: Also, if we adopt URP in AWSIM, these camera functions will be affected.

@lchojnack
Copy link
Contributor Author

lchojnack commented May 15, 2024

@mackierx111

Is it correct that the main purpose is to reproduce C1 and C2 cameras using volume profiles and HDAddtionalCameraData component?

The main purpose is to implement C1 and C2 cameras (models, prefabs) and some camera features. Most of the selected camera features are available in Unity using Volume Profiles. The sharpness and Lens Distortion Correction was implemented using Compute Shaders. More details can be found in the report here

Am I correct that CameraSensorFeatureManager is a class that sets HDAddtionalCameraData and Volume/VolumeProfile from a script?
Is there a problem with setting HDAddtionalCameraData and Volume/VolumeProfile without CameraSensorFeatureManager?

Yes, CameraSensorFeatureManager is a class that sets the volume or camera parameters. Basically, there is no problem setting the volume or camera parameters manually without the CameraSensorFeatureManager script. However, the CameraSensorFeatureManager is designed for ease of use. It acts as an interface for the user to enable/disable and control the camera effects.
The problem with controlling camera effects without the CameraSensorFeatureManager is that some effects are accessed in the volume object, while others are accessed in the camera object. There are also effects, such as motion blur, that require the camera object to be enabled. The intention of this CameraSensorFeatureManager is to handle all this for the user.

What are your thoughts on integrating the CameraSensorFeatureManager and CameraSensor classes?

This is a question of design. We decided to have two separate scripts. But there is nothing stopping us from merging the CameraSensorFeatureManager into CameraSensor. The question is if all camera sensors will have the image effects implemented? Also, CameraSensor already does a lot, so adding additional functionality might make CameraSensor less readable.

Can you please add documentation on the C1, C2 camera or this camera implementation?

Yes, we can add documentation. Can we merge this PR first and add the documentation in a separate PR?

Copy link
Collaborator

@mackierx111 mackierx111 left a comment

Choose a reason for hiding this comment

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

Thanks for the implementation. I have added a few comments on my end. LGTM!

@mackierx111 mackierx111 merged commit e2255df into main May 27, 2024
1 of 2 checks passed
@mackierx111 mackierx111 deleted the feature/c1_c2_cameras branch May 27, 2024 07:57
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