The “uniq” method is a very good way to remove duplicate string characters in a “Array”.
I find myself in a situation where the method does not work.
def creer_et_remplacer_materiaux_habillages(ents)
mod = Sketchup.active_model
mat = mod.materials
mod.start_operation('mataddpn', true)
ents.grep(Sketchup::ComponentInstance).each do |e|
["Exploser"].each do |ajouter|
if e.definition.name =~ /#{ajouter}/
if e.material.name.include?"PN"
mathab = []
mathab << "#{e.material.name}-#{e.definition.name}"
mathab2 = mathab.uniq
puts mathab2
path = File.join(Sketchup.find_support_file('plugins'),
"TNT_ClickCuisine2/Materials/FACADES/"
)
for th in mathab2
mathab2.each do |mh|
mattex = "4-#{mh.split("-")[0]}.jpg"
math = mat.add(mh)
math = mat[th]
math.texture = path + mattex
e.material = math
end
end
end
end
end
creer_et_remplacer_materiaux_habillages(e.definition.entities)
end
mod.commit_operation
end
mod = Sketchup.active_model
ent = mod.active_entities
sel = mod.selection
instances = sel.grep(Sketchup::ComponentInstance)
creer_et_remplacer_materiaux_habillages(instances)
If you used correct indentation it would maybe be obvious — and meaningful variable names improve readability and avoid bugs.
As far as I see, mathab is created as an array and immediately only one element is added. It never contains more than one element so it is always unique.
If there is still a problem in your code then you need to think again what you print to the console for debugging. Why not e.definition.name and mathab.inspect?
It’s very hard to figure out what is the intended behavior of the code when it’s written partly in French. I have no idea what “mathab”, “mathab2”, “math”, “mataddpn” or “creer_et_remplacer_materiaux_habillages” is supposed to mean. Also in my experience abbreviation such as mod, mat, sel and e, although common, makes difficult to read code. If you instead write the full word model, materials, selection and entity it is much easier to understand what it refers to.
As Aerilius has already said correct indentation would also help a lot.
Computers pair up if, do, unless and certain other keywords with a corresponding end keyword to understand the hierarchy of the code. Humans rely on indentation. There is no way to read this code without first copying it into a code editor and fix the indentation.
See this example:
def method_name
# This code runs whenever the method method_name is called.
if some_statement
# This code runs whenever the method is called AND some_statement evaluates to true.
end # This ends the if.
5.times do
# This code runs 5 times whenever the method is called.
if some_other statement
# This code runs a maximum of 5 times and only while some_other_statement evaluates to true.
end # this ends the if.
end # This ends the do loop.
end # This ends the method definition.
If you indent the code arbitrarily it’s impossible to understand what code runs when and why, without carefully pairing up the keywords yourself.
def method_name
# This code runs whenever the method method_name is called.
if some_statement
# This code runs whenever the method is called AND some_statement evaluates to true.
end # Okay?
5.times do
# So when does this run? Does it depend on some_statement? How many times does it run?
if some_other statement
# Headache.
end
end # What code block ends here?
# Wait, what? Why is there an indentation level missing here?
end
To go back to the original question, the Array is made unique but that doesn’t make any difference as it only contains one value. To get a bigger Array containing the data from several component instances, the Array needs to be created before you iterate over the entities collection. Then you can uniq it after the loop. All this gets much clearer with correct indentation.
Sorry, it is (almost) perfectly indented, but Firefox renders the indentation with a wrong tabstop width. Use consistently either only tabstops or only spaces but don’t mix them. Take also a look in your text editor’s settings and set it to use tabstop width = 2 and to replace tabstops by spaces.
Most important is consistency. Methods usually have parentheses, these can be used to distinguish them from variables. If you abbreviate common variables that is ok as long as there is a consistent scheme behind to avoid misunderstandings (see “math”). Abbreviated code does not run faster (it’s not assembler, and the compiler/interpreter is responsible for optimizations). Code must be good for humans.
Before we can fix the code, we (and you too) need to have an idea what the fixed code should achieve. There are two redundant for/each loops but I don’t think you want to do a square iteration (all combinations of mathab strings with other mathab strings). There is recursion that calls a UI method (nested start_operation!), but UI code and domain code should be separated (at least separate methods).
Before drafting again an algorithm, you need a clear specification. Can you describe in phrases what it is supposed to achieve?
I didn’t notice tabs and spaces were mixed! Mixing is no good idea as there is no universally defined tabstop. Some systems/editors use 2, some 4 and some 8. Sticking to spaces only is the safest and most editors have a setting for this.
Regarding variable naming using abbreviation doesn’t help separate to them from methods as methods could also be named using abbreviation. Also, there are no methods named model, selection or material in the namespace used, so there is no risk of a mix-up.
Here’s my edited code with the following changes:
indentation
using full words instead of abbreviation (where i understand what it is supposed to mean),
using placeholder variable names var1, var2 etc where I don’t understand what is is supposed to mean,
using placeholder variable and method names for everything in French and
using placeholder operation name.
def french1(entities)
model = Sketchup.active_model
materials = model.materials
model.start_operation('Operator Name', true)
entities.grep(Sketchup::ComponentInstance).each do |entity|
["Exploser"].each do |french2|
if entity.definition.name =~ /#{french2}/
if entity.material.name.include?("PN")
var1 = []
var1 << "#{entity.material.name}-#{entity.definition.name}"
var2 = var1.uniq
puts var2
path = File.join(
Sketchup.find_support_file('plugins'),
"TNT_ClickCuisine2/Materials/FACADES/"
)
for var3 in var2
var2.each do |var4|
mattex = "4-#{var4.split("-")[0]}.jpg"
var5 = materials.add(var4)
var5 = materials[var3]
var5.texture = path + mattex
entity.material = var5
end
end
end
end
end
french1(entity.definition.entities)
end
model.commit_operation
end
model = Sketchup.active_model
selection = model.selection
instances = selection.grep(Sketchup::ComponentInstance)
french1(instances)
You can change all the placeholder names to meaningful names that tell what a method does and what a variable represents. I just also noticed that mattex probably should be changed to file_name as it seems to represent a file name (not a material or texture or whatever mattex even refers to).
It is very hard for us here in the forum to understand what the code is even supposed to do when we don’t understand what variables are supposed to represent. It will also be equally hard for you to understand these names if you need to make any changes to the code in a few years from now when you no longer have it fresh in memory.
But almost perfect is not enough, it must be perfect!
Thank you eneroth3, for the effort you made so that I can understand you.
Indeed the indentation was not perfect and I would use your example for the next methods.
I agree with you, writing explicit variables undoubtedly improves understanding of the code.
However it is easier for me to write variables in French and my codes will always be difficult to read for you eneroth3.
Is it really insurmountable for you to understand a code with variables in another language?
In your example, “var1”, “var2”, “var3”, is not simpler than “mathab”, “mathab2”, “mattex” …
To conclude,
I start with the principle that we must always question ourselves to make progress even if our experience seems long and wonderful.
They are equally hard to understand, but the left example is honest about being incomprehensible while the right is just cryptic. That said var1, var2 and var3 should of course be changed into something that explains what they are.
mathab doesn’t in any way tell what the variable represents. It looks like it is related to math or materials but it could just as well be a phone number or the distance between the earth and the moon. You just can’t tell from looking at it.
Given a set of entities, find all contained component instances (recursively). If the instance's definition and material match a pattern, replace the instance material by a new material that is unique to the component definition and uses a texture image from an accompagnied resources folder.
To your approach:
(I guess) you actually wanted to avoid creating duplicate materials. Your approach was to iterate over entities to add material names (tuples of material&definition) to `mathab`. Then make the names unique, then create a material for each name, then iterate again over the entities to assign the material. This requires one loop after another, not nested loops.
An alternative strategy for avoiding duplication is to check existence (with include?) instead of using unique.
# Define specific strings and numbers as constants so that they are not spread over the code.
COMPONENT_PATTERNS = [/Exploser/]
MATERIAL_PATTERN = 'PN'
PATH_FACADES = File.join(Sketchup.find_support_file('plugins'), 'TNT_ClickCuisine2/Materials/FACADES/')
def do_create_and_replace_cover_materials_operation(entities)
mod = Sketchup.active_model
mod.start_operation('mataddpn', true)
create_and_replace_cover_materials(entities)
mod.commit_operation
end
# Make every component instance's material unique for the component definition.
def create_and_replace_cover_materials(entities, materials_cache={}, visited_definitions=[])
entities.grep(Sketchup::ComponentInstance).each do |e|
if COMPONENT_PATTERNS.any?{ |pattern| e.definition.name =~ pattern } # Run the block below only once if any of the patterns matches, not again for every match.
if e.material.name.include?(MATERIAL_PATTERN) # Use parentheses
# Make the instance's material unique for this definition.
# To avoid duplicates, materials are created only once and reused from a cache.
# They are identified by the pair of instance material and definition.
if !materials_cache.include?([e.material, e.definition])
materials_cache[[e.material, e.definition]] = create_cover_material(e.material, e.definition)
end
e.material = materials_cache[[e.material, e.definition]]
end
end
# Recursion over nested components.
# Run the recursion only once per definition, not again for every instance that has the same definition.
if !visited_definitions.include?(e.definition)
visited_definitions << e.definition
create_and_replace_cover_materials(e.definition.entities, materials_cache, visited_definitions)
end
end
end
# Create a single new material based on an existing material and a definition.
def create_cover_material(instance_material, definition)
material_name = "#{instance_material.name}-#{definition.name}"
material = instance_material.model.materials.add(material_name) # Entities have a reference to their model, no need to pass a model reference as parameter, no need to assume it is the active model.
texture_filename = "4-#{instance_material.name}.jpg" # String splitting (or other operations) require assumptions on how the string is constructed, assumptions cause bugs.
material.texture = File.join(PATH_FACADES, texture_filename) # Use File.join, not +
return material
end
mod = Sketchup.active_model
ent = mod.active_entities
sel = mod.selection
instances = sel.grep(Sketchup::ComponentInstance)
do_create_and_replace_cover_materials_operation(instances)
Both SketchUp forums (here and SketchUcation) do not format code well when you paste in text with embedded TAB characters. The Discourse forum engine (used here) has an 8 character default for TABs. So it moves all your code way over to the right, and causes unneeded horizontal scroll bars (and scrolling by those reading the code.)
It is assumed that any sample code given in the forum, will be wrapped within YOUR plugin sub-module namespace, that should be wrapped within YOUR author/company toplevel module namespace.
So, NO there will be no clashes between constants defined within YOUR namespaces (class or modules) and other people’s constants defined within THEIR namespaces (ie, classes or modules.)
Custom classes should only be defined at the toplevel by Ruby, (and in rare instances the SketchUp API,) and are meant for access by ALL extensions.
Before drafting again an algorithm, you need a clear specification. Can you describe in phrases what it is supposed to achieve?
