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

feat(upvector): Add upvector Radiance property #627

Closed
wants to merge 0 commits into from

Conversation

mikkelkp
Copy link
Member

No description provided.

Copy link
Member

@chriswmackey chriswmackey left a comment

Choose a reason for hiding this comment

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

Nothing here is a breaking change but I left some comments since I am a little unclear why we need an up_vector property that is different from that associated with the BSDF modifier.

@@ -44,6 +45,24 @@ def __init__(self, host, modifier=None, modifier_blk=None,
_DynamicRadianceProperties.__init__(
self, host, modifier, modifier_blk, dynamic_group_identifier)

@property
def upvector(self):
"""Get or set the object modifier."""
Copy link
Member

Choose a reason for hiding this comment

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

Hey @mikkelkp . I guess the docstring is wrong here.

Also, I just want to make sure I am understanding this correctly: we need to have an up_vector here that is separate from the up_vector that is assigned to the BSDF modifier that the aperture has?
https://github.com/ladybug-tools/honeybee-radiance/blob/master/honeybee_radiance/modifier/material/bsdf.py#L146-L168

So there are two up_vectors that each can be different from one another?

Copy link
Member

Choose a reason for hiding this comment

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

So there are two up_vectors that each can be different from one another?

This is one of those confusing concepts in Radiance and rfluxmtx. They are not the same. The upvector for BSDF is the upvector of the BSDF material. The upvector for receiver is the upvector of the geometry. Radiance uses the up-vector of the geometry to map/align the BSDF file on top of the aperture.

Here are my notes from the Radiance BSDF workshop:

image

image

Also see rfluxmtx documentation: https://www.radiance-online.org/learning/documentation/manual-pages/pdfs/rfluxmtx.pdf/view

if value is not None:
assert isinstance(value, Vector3D), \
'Expected Ladybug Vector3D. Got {}'.format(type(value))
self._upvector = value
Copy link
Member

Choose a reason for hiding this comment

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

This is a duplication of what you added to the Aperture class. Can we just remove this here?

Or can you make it so that the up_vector behaviour is the same across all of these _base classes?

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.

None yet

3 participants