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

Adding SpotLight to Lighting kit #219

Merged
merged 6 commits into from
May 6, 2022
Merged

Conversation

MrXandbadas
Copy link
Contributor

Added the spot_blend and spot_size parameters into the lights.py file.

Spot Blend is the roughness of the beam
Spot Size is the radius in deg 0 - 180 # This might be incorrect?

Blender.py was changed to add the needed parameters inside the blender handler. I think I added the correct identifiers and what not. I just copied the standard implementation as it was written.

The SpotLight was added in with the other forms of Lights


Personal Note:
Still need to look at the colors of the lights. Maybe refer to the issue in the main github of Kubric regarding the Color Class and its implementation and look at reworking for a more wide-spread color use. Need to look into the color implementation.

Added the spot_blend and spot_size parameters into the lights.py file.

Spot Blend is the roughness of the beam
Spot Size is the radius in deg 0 - 180

Blender.py was changed to add the needed parameters inside the blender handler. I think I added the correct identifiers and what not. I just copied the standard implementation as it was written.

The SpotLight was added in with the other forms of Lights

----------
Still need to look at the colors of the lights. Maybe refer to the issue in the main github of Kubric regarding the Color Class and its implementation.
Copy link
Collaborator

@Qwlouse Qwlouse left a comment

Choose a reason for hiding this comment

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

Two minor comments, otherwise LGTM.
Thanks for adding this!

kubric/core/lights.py Outdated Show resolved Hide resolved
@@ -474,6 +474,22 @@ def _add_asset(self, obj: core.DirectionalLight): # pylint: disable=function-re
obj.observe(KeyframeSetter(sun, "energy"), "intensity", type="keyframe")
return sun_obj

def _add_asset(self, obj: core.SpotLight): # pylint: disable=function-redefined
SpotLight = bpy.data.lights.new(obj.uid, "SPOT")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: lowercase variable names please (snake case)

@MrXandbadas
Copy link
Contributor Author

#225 has been addressed in the PR as it is a one liner

After rebuilding the shapelib container and starting the parfor script, I observed something off with the archives: Screenshot from 2022-04-26 18-33-49

There are 2 files with the same name.

When extracting, one of the files overwrites the other one.

Here are the offending lines:

tar.add(object_folder / 'kubric' / 'collision_geometry.obj',
arcname='collision_geometry.obj')
tar.add(object_folder / 'kubric' / 'model_watertight.obj',
arcname='collision_geometry.obj')

I suppose the second file should have arcname='model_watertight.obj', correct? I can't find documentation for the specific format this is converting to; I assume it's using the same format as the MOVi objects?

@MrXandbadas MrXandbadas marked this pull request as ready for review April 26, 2022 16:27
@MrXandbadas MrXandbadas requested a review from Qwlouse May 1, 2022 10:22
Copy link
Collaborator

@Qwlouse Qwlouse left a comment

Choose a reason for hiding this comment

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

two more bugfixes :-)

kubric/core/lights.py Show resolved Hide resolved
kubric/renderer/blender.py Outdated Show resolved Hide resolved
MrXandbadas and others added 2 commits May 7, 2022 01:02
Bug Fix noted by Qwlouse
@MrXandbadas MrXandbadas requested a review from Qwlouse May 6, 2022 15:07
@Qwlouse Qwlouse merged commit 09e4047 into google-research:main May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants