Hi @REDO_79, nice work!
I already gave you some advice here
I see that you went through the dictionary route, but didn’t catch the rest of the advice, so here we are…
By putting the logic inside the Window subclasses, you have coupled the GUI with the logic, and while this is fine for a simple, single use case, it doesn’t allow you to reuse the code in another fashion, for example via the pyrevit CLI or via a route endpoint.
Also, in the unlikely event that WPF is no longer supported by new versions of Revit, you would have to rewrite everything instead of only the graphic part.
Some tips to avoid this coupling:
- the methods that don’t access
selfinside them can be quickly moved out of the class an made into functions; - after that, some of the remaining methods only access
self.data, and they are called aftercollect_input_data, so just passdataas argument, and they also become simple functions to move out of the class!
You now should have the classes that only load the XML, initialize the data, handle the clicks and reads the input data.
And since the OK_Clicked method will mostly call external function, you might just turn into a method that collects the input data and closes the window if there is no error, then move the rest of the logic to a main function.
Refactored window class
class ManholeRebar(Window):
def __init__(self):
xaml_path = os.path.join(os.path.dirname(__file__), 'ferraillage.xaml')
wpf.LoadComponent(self, xaml_path)
# no need to save the list in a separate attribute that it not used anywhere...
self.select_rebar.ItemsSource = [10, 12, 14, 16, 20, 25, 32, 40]
self.data = {}
self.ShowDialog()
def _collect_input_data(self):
"""Collect and convert input data from the UI"""
# for this particular case, its seems clearer to me to check things beforehand
# rather than wrapping everything in a try/except
if not self.e1_value.Text:
forms.alert("Please enter a valid value for : e1")
elif not self.e2_value.Text:
forms.alert("Please enter a valid value for : e2")
elif not self.spacing_value.Text:
forms.alert("Please enter a valid value for : s")
elif self.select_rebar.SelectedItem is None:
forms.alert("Please select a rebar diameter: d")
else:
try:
self.data = {
"E1": _input_to_centimeters(self.e1_value.Text),
"E2": _input_to_centimeters(self.e2_value.Text),
"S": _input_to_centimeters(self.spacing_value.Text),
"d": self.select_rebar.SelectedItem
}
except ValueError:
forms.alert(
"Invalid input detected. "
"Please ensure all inputs are numeric where required."
)
def OK_Clicked(self, sender, e):
self._collect_input_data()
if not self.data:
return
self.Close()
def Cancel_Clicked(self, sender, e):
self.Close()
# extracted out the repeating code from the collect input data method
def _input_to_centimeters(value):
"""Convert a textual value to centimeters."""
return UnitUtils.ConvertToInternalUnits(float(value), UnitTypeId.Centimeters)
Next, you load all sort of things before loading the input form, but if the user clicks cancel you have wasted time and memory for nothing. Collect the walls, floors and rebar types only when needed.
Also, using globally scoped variables (the one you declared before the form class) inside functions is not a great idea if you want to make them more “atomic”, that is, to be able to run them from other scripts. Let’s try to fix that.
Since you use only the first floor element that you find, you can retrieve it directly with FirstOrDefault() (returns None if no floors are found) First(), (raises an error if no floors are found) or Single() (raises an error if no or more than one floors are found).
def get_first_floor(doc):
"""Return the first floor found in the document."""
return (
FilteredElementCollector(doc).OfCategory(BuiltInCategory.OST_Floors)
.WhereElementIsNotElementType()
.ToElements()
.FirstOrDefault()
)
NOTE: As you can see, I use the
docargument to refer to the document, to be able to use the function on other documents and not only the active one; we’ll see how it can be done later.
I’ll do this to all the functions that needs access to the document.
The rebar types list is only used in the rebar_Type function, so you can embed it there.
Also, to find a single element in a sequence, use next to speedup the process, since it will stop at the first matching item instead of looping through all the list to just pick one item.
I also believe Element.Name.GetValue(r) could be simplified to just r.Name… but I have not tested it.
def get_rebar_type(diameter, doc):
"""Return the rebar type associated to the given diameter."""
# we only need the rebar types collection here, so I moved it from the global scope
rebar_name = "HA{} (Fe400)".format(diameter)
rebar_types = (
FilteredElementCollector(doc).OfCategory(BuiltInCategory.OST_Rebar)
.WhereElementIsElementType()
.ToElements()
)
return next(r for r in rebar_types if r.Name == rebar_name)
NOTE: I’ve renamed this and the other functions to be more explanatory, see the naming style comments below
You can also use the LinQ .Where method to do it all at once
def get_rebar_type(diameter, doc):
"""Return the rebar type associated to the given diameter."""
# we only need the rebar types collection here, so I moved it from the global scope
rebar_name = "HA{} (Fe400)".format(diameter)
return (
FilteredElementCollector(doc).OfCategory(BuiltInCategory.OST_Rebar)
.WhereElementIsElementType()
.Where(lambda r: r.Name == rebar_name)
.FirstOrDefault()
)
The units variable is used only in the covers function, so it can be moved inside there.
def get_covers_parameters(data, floor_width, doc):
"""Cover parameters."""
units = doc.GetUnits().GetFormatOptions(SpecTypeId.Length).GetUnitTypeId()
c1 = width/ 2 + data["E1"]
c2 = floor_width - data["E2"]
d2 = c2 - UnitUtils.ConvertToInternalUnits(data["d"], UnitTypeId.Millimeters)
c3 = UnitUtils.ConvertToInternalUnits(0.05, units)
return c1, c2, d2, c3
Note on the cover variable: you pass it as is to the various functions, but then use only a subset of the items; I suggest to be more explicit and specify the exact parameters that a function needs and only pass those parameters.
Here’s what your core function could look like:
def create_manhole_rebars(type, cover, floor, step, doc):
"""Create the rebars for a manhole."""
# functions are now self explanatory, no need for comments at each line
c1, c2, d2, c3 = cover
curveloop1 = create_vertical_curveloop(walls[0], c1, c2, d2)
path1 = create_vertical_rebar_path_distribution(walls[0], c1)
curveloop2 = create_vertical_curveloop(walls[1], c1, c2, d2)
path2 = create_vertical_rebar_path_distribution(walls[1], c1)
curveloop3 = create_horizontal_curveloop(walls, c3)
path3 = create_horizontal_rebar_path_distribution(walls[0], c3)
loop1 = translate_curveloop_along_path(curveloop1, path2, step)
loop2 = translate_curveloop_along_path(curveloop2, path1, step)
loop3 = translate_curveloop_along_path(curveloop3, path3, step)
# create rebars
with Transaction(doc, 'create rebars') as t:
t.Start()
validationResult = clr.Reference[RebarFreeFormValidationResult]()
Rebar.CreateFreeForm(doc, type, floor, loop1, validationResult)
Rebar.CreateFreeForm(doc, type, floor, loop2, validationResult)
Rebar.CreateFreeForm(doc, type, floor, loop3, validationResult)
t.Commit()
The hardest thing for me to undestand and refactor is the walls variable, but I’ve invested too much time already on this review, so I’ll leave it to you.
The only thing I will point out is that the V_path function is nonsense to me: you pass a wall as argument to then check against the first item in the walls list to then use the other wall… just pass the other wall and it’s done! (I already changed the arguments in the core function above)
def create_vertical_rebar_path_distribution(other_wall, c1):
"""Define path for vertical rebar distribution."""
temp_line = other_wall.Location.Curve
p_a = c1 / temp_line.Length
return Line.CreateBound(
temp_line.Evaluate(p_a, True),
temp_line.Evaluate(1 - p_a, True)
)
Putting it all together, the main function goes like this
def main(doc=None):
"""Main entrypoint for the script."""
doc = doc or revit.doc # revit can be imported from pyrevit
form = ManholeRebar()
if not form.data:
return
data = form.data
# see the advices below to understand these changes
type = get_rebar_type(data["d"], doc)
floor = get_first_floor(doc)
floor_width = floor.get_Parameter(BuiltInParameter.FLOOR_ATTR_THICKNESS_PARAM).AsDouble()
cover = get_covers_parameters(data, floor_width, doc)
create_manhole_rebars(type, cover, floor, data["S"], doc)
if __name__ == "__main__":
main()
The last two lines allows you to run the main function on the active document if the script is run via the button click, but you can also import the main function in another script and call it by passing whatever document you want with main(my_document).
Other tips:
typeis a reserved word, don’t use it as variable name- In c# the naming standard is PascalCase for classes and methods and camelCase for variables and method arguments; Python uses PascalCases for classes and snake_case (lowercase) for almost everything else, you’re mixing the two styles; not an error, but it hurts my eyes
a general advice is to stick with one of the two styles through all your code, your future self will thank you. Obviously this is for the classes/methods/variables you create/define. - method names should be of the form
verb_noun, likeget_covers_parameters, and in general the naming should be self explanatory instead of having to explain it with a comment - instead of using comments above the function definition, use docstrings inside them