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

Refactor face_attr and room_attr inputs #27

Merged
merged 8 commits into from
Oct 10, 2023
Merged

Conversation

mostaphaRoudsari
Copy link
Member

This PR introduces new objects for face and room attributes to allow mixing different combinations between different attributes.

This is a BREAKING CHANGE as it changes the input arguments for the function. I didn't try to fix the tests at this point because I wanted to make sure that @chriswmackey is fine with introducing these changes first.

Here is an example where I create two face attributes objects. The first face attribute colors the faces based on the user_data.tag and the other one writes the layer name using user_data.__layer__.

from honeybee.model import Model, Aperture
from honeybee_display.model import model_to_vis_set, FaceAttribute, RoomAttribute
from ladybug_display.visualization import VisualizationSet
from ladybug_vtk.visualization_set import VisualizationSet as VTKVS

fp = r"classroom_tagged_aperture.hbjson"
model = Model.from_hbjson(fp)

fa_1 = FaceAttribute(
    name='Aperture_Tags',
    attr=['user_data.tag'], color=True,
    face_types=[Aperture]
)

fa_2 = FaceAttribute(
    name='Aperture_Layers',
    attr=['user_data.__layer__'], color=False, text=True,
    face_types=[Aperture]
)

vs: VisualizationSet = model_to_vis_set(model=model, face_attr=[fa_1, fa_2])

vvs = VTKVS.from_visualization_set(vs)
vvs.to_html(name='glass_model')

Here are the input and output files.

test_files.zip

The new attributes also allow filtering the objects by type. For this example, I'm only interested in the apertures.

image

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.

Thanks, @mostaphaRoudsari .

I'm generally in favor of the change but I'm mostly concerned about the impact it will have on the CLI command. Do you think you could implement this on the CLI in a way that did not remove any of the existing options on the command? This way, we don't need to involve Konrad (and I think also Mikkel and maybe Antonello) in the change here.

Basically, I'm imagining that these detailed specifications would be supplied to the command as a new JSON file option. And you keep all of the existing command options such that you format them into these RoomAttribute and FaceAttribute objects within the command, adding them to the detailed JSON specification if it's supplied.

It seems that would allow us to get the best of both worlds with the existing capability of just specifying the attributes we want within the command or we could just write out a whole JSON of RoomAttribute and FaceAttribute objects if we want more control.

honeybee_display/attr.py Show resolved Hide resolved
This commit adds a new `face_attr_types` input to the `model_to_vis_set` function. This argument is in particular useful for only coloring Apertures or Floors to represent information that are limited to apertures or room-based information without coloring the whole room.

I understand that Aperture and Shade are not the same as other FaceTypes but for a developer they can be considered as their own face type.
@mostaphaRoudsari mostaphaRoudsari merged commit b8237d9 into master Oct 10, 2023
16 checks passed
@github-actions
Copy link

🎉 This PR is included in version 0.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mostaphaRoudsari mostaphaRoudsari deleted the refactor-attrs branch October 11, 2023 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants