Changeset View
Standalone View
tests/python/modules/object_generator.py
- This file was added.
| # ##### BEGIN GPL LICENSE BLOCK ##### | |||||
| # | |||||
| # This program is free software; you can redistribute it and/or | |||||
| # modify it under the terms of the GNU General Public License | |||||
| # as published by the Free Software Foundation; either version 2 | |||||
| # of the License, or (at your option) any later version. | |||||
| # | |||||
| # This program is distributed in the hope that it will be useful, | |||||
| # but WITHOUT ANY WARRANTY; without even the implied warranty of | |||||
| # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | |||||
| # GNU General Public License for more details. | |||||
| # | |||||
| # You should have received a copy of the GNU General Public License | |||||
| # along with this program; if not, write to the Free Software Foundation, | |||||
| # Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | |||||
| # | |||||
| # ##### END GPL LICENSE BLOCK ##### | |||||
| # <pep8 compliant> | |||||
| # Inspired by mesh_test | |||||
| import bpy | |||||
| import os | |||||
| class TestObject: | |||||
| """ | |||||
| To hold specs to create a test object | |||||
| """ | |||||
| def __init__(self, collection_name, obj_dict, count=1): | |||||
zazizizou: This class does way more than just holding specs. It creates the test object with an offset too. | |||||
| """ | |||||
| :param collection_name: str - name of the collection, also acts a unique test name | |||||
Not Done Inline ActionsWhy not use the a similar interface to OperatorSpec for selection? zazizizou: Why not use the a similar interface to `OperatorSpec` for selection?
| |||||
| :param obj_dict: dict - key/value pair for object name and an attribute(call) to it. | |||||
| :param count: int - to make duplicate copies of it. | |||||
| """ | |||||
| self.collection_name = collection_name | |||||
| self.obj_dict = obj_dict | |||||
| self.count = count | |||||
| class GenerateTestObject: | |||||
| """ | |||||
| Generates TestObject | |||||
| """ | |||||
| def __init__(self, path, filename, test_object_list): | |||||
| """ | |||||
| :param path: str - path/to/the/file excluding the filename | |||||
| :param filename: str - name of the blend file | |||||
| :param test_object_list: list - list of objects we want to create TestObject. | |||||
| """ | |||||
| self.path = path | |||||
| self.filename = filename | |||||
| self.path_to_file = os.path.join(self.path, self.filename) | |||||
| self.is_new_file = False | |||||
Not Done Inline ActionsWhy? you can always extract the directory from the full path and therefore simplify the interface zazizizou: Why? you can always extract the directory from the full path and therefore simplify the… | |||||
| self._check_for_valid_path() | |||||
| self._hide_camera_and_light() | |||||
| self.test_object_list = test_object_list | |||||
| self.list_of_added_objects = [] | |||||
| def _check_for_valid_path(self): | |||||
| """ | |||||
| Checks if the path is valid and if the file exists or not. | |||||
| """ | |||||
| if os.path.exists(self.path): | |||||
| if os.path.exists(self.path_to_file): | |||||
| bpy.ops.wm.open_mainfile(filepath=self.path_to_file) | |||||
| else: | |||||
| self.is_new_file = True | |||||
| bpy.ops.wm.save_as_mainfile(filepath=self.path_to_file) | |||||
| print("No existing file found. New file created {}".format(self.filename)) | |||||
| else: | |||||
| raise Exception("Incorrect path for the file") | |||||
| def _hide_camera_and_light(self): | |||||
| # Cleaning up a new blend file | |||||
| bpy.data.objects['Light'].hide_set(True) | |||||
Done Inline Actionslist_of_added_objects is a parameter that GenObject should take and and not TestObject itself, because as mentioned above, GenObject should be responsible for coordinating TestObject class instances zazizizou: `list_of_added_objects` is a parameter that `GenObject` should take and and not `TestObject`… | |||||
| bpy.data.objects['Camera'].hide_set(True) | |||||
| def create(self): | |||||
| """ | |||||
| Generates objects present in the list. | |||||
Not Done Inline ActionsPrefer more specific exceptions, e.g. NotADirectoryError or FileNotFoundError zazizizou: Prefer more specific exceptions, e.g. `NotADirectoryError` or `FileNotFoundError` | |||||
| """ | |||||
| try: | |||||
| for test_object in self.test_object_list: | |||||
| if isinstance(test_object, TestObject): | |||||
| self.create_obj(test_object.count, test_object.obj_dict, test_object.collection_name) | |||||
| bpy.ops.wm.save_mainfile(filepath=self.path_to_file) | |||||
| except: | |||||
| if self.is_new_file: | |||||
| os.remove(self.path_to_file) | |||||
| print("The {} file was deleted!".format(self.filename)) | |||||
| raise Exception("Something went wrong!") | |||||
| def create_obj(self, count, obj_dict, collection_name): | |||||
| """ | |||||
| Calls for _create_object function for count of objects. | |||||
| """ | |||||
| for index_of_object in range(count): | |||||
| self._create_object(index_of_object, obj_dict, collection_name) | |||||
| def _create_object(self, index, obj_dict, collection_name): | |||||
| """ | |||||
| Create a test object and expected object. | |||||
Not Done Inline ActionsThis is not helpful. It's better to re-raise the original exception. zazizizou: This is not helpful. It's better to re-raise the original exception. | |||||
| :param index: to name the objects test/exp objects accordingly, e.g. testObj0, testObj1 etc | |||||
| """ | |||||
| offset_y = self._get_last_location() + 5 | |||||
Not Done Inline Actionsthis looks like a cleanup function that should be called in the destructor of GenObject zazizizou: this looks like a cleanup function that should be called in the destructor of `GenObject` | |||||
Done Inline ActionsNot very clear how a destructor would be called in here. calra: Not very clear how a destructor would be called in here. | |||||
Not Done Inline ActionsCan you explain the responsibility of the class GenerateTestObject? Does it not necessarily generate test objects? The original idea was to define a class that specifies how an object should be created, e.g. TestObject and another one that actually creates the object, i.e. GenerateTestObject, so any property of the test object (selection, vertex group, etc..) should be stored in TestObject zazizizou: Can you explain the responsibility of the class `GenerateTestObject`? Does it not necessarily… | |||||
Done Inline ActionsYes but it also generates additional supporting for test objects. Hmm, I see. Giving Vertex Group, a separate class would allow for less dependence. Adding vertex groups to already existing objects. Also I can't recall what exactly is meant by selection here? (Is it related to selecting vertices in a vertex group?) Also having so many optional parameters would complicate the interface, no? calra: Yes but it also generates additional supporting for test objects.
Hmm, I see. Giving Vertex… | |||||
Not Done Inline ActionsSelection is useful for operators because they operate on selection and not on the whole mesh. For the interface making most of the parameters optional should solve it. See how bpy.ops.wm.save_as_mainfile has so many parameters and it's still easy to use because you usually only set those that you need. zazizizou: Selection is useful for operators because they operate on selection and not on the whole mesh. | |||||
| for obj_name, obj_type in obj_dict.items(): | |||||
| test_obj_name = "testObj" + obj_name + str(index) | |||||
| if test_obj_name not in bpy.data.objects.keys(): | |||||
| try: | |||||
| getattr(bpy.ops.mesh, obj_type) | |||||
| mesh_obj = getattr(bpy.ops.mesh, obj_type) | |||||
| print("{} created successfully.".format(test_obj_name)) | |||||
| except AttributeError: | |||||
Not Done Inline Actionsoutdated doc zazizizou: outdated doc | |||||
| raise AttributeError("Incorrect parameter: {} for adding object.".format(obj_type)) | |||||
Not Done Inline ActionsAssigning scene_name here seems redundant, assigning scene = bpy.context.scene is enough, no need for lookups to bpy.data.scenes after that. campbellbarton: Assigning `scene_name` here seems redundant, assigning `scene = bpy.context.scene` is enough… | |||||
| mesh_obj(location=(0, offset_y, 0)) | |||||
| bpy.context.active_object.name = test_obj_name | |||||
Done Inline ActionsThis looks like the wrong place for such a message. It should be shown where the object was actually created and not when it's about to get removed from the default collection zazizizou: This looks like the wrong place for such a message. It should be shown where the object was… | |||||
| bpy.ops.object.duplicate_move(TRANSFORM_OT_translate={"value": (5, 0, 0)}) | |||||
| exp_obj_name = "expObj" + obj_name + str(index) | |||||
| bpy.context.active_object.name = exp_obj_name | |||||
| print("{} created successfully.".format(exp_obj_name)) | |||||
| self._link_to_collection(test_obj_name, exp_obj_name, index, collection_name) | |||||
| offset_y += 5 | |||||
Done Inline ActionsI think you should take care of correct linking where you create the object. It really seems weird that a TestObject instance cleans up or unlinks other object it might not know about. zazizizou: I think you should take care of correct linking where you create the object. It really seems… | |||||
| else: | |||||
| print("Object already present.") | |||||
Done Inline ActionsGenObject -> GenerateTestObjects zazizizou: `GenObject` -> `GenerateTestObjects` | |||||
| self._unlink_from_unwanted_collection(collection_name) | |||||
Done Inline ActionsThis class also does more than that, but it's fine. All test and file creation should happen in this class zazizizou: This class also does more than that, but it's fine. All test and file creation should happen in… | |||||
| def _link_to_collection(self, test_obj_name, exp_obj_name, index, collection_name): | |||||
| """ | |||||
| Add test and expected objects to a Collection | |||||
| """ | |||||
| # Maintaining a list of all test and expected objects. | |||||
| test_obj = bpy.data.objects[test_obj_name] | |||||
| self.list_of_added_objects.append(test_obj) | |||||
| exp_obj = bpy.data.objects[exp_obj_name] | |||||
| self.list_of_added_objects.append(exp_obj) | |||||
| collection = bpy.data.collections.new(name=collection_name) | |||||
| collection.name = collection_name + str(index) | |||||
Done Inline ActionsUse os.path.join. campbellbarton: Use `os.path.join`. | |||||
| bpy.context.scene.collection.children.link(collection) | |||||
| collection.objects.link(test_obj) | |||||
| collection.objects.link(exp_obj) | |||||
| def _get_last_location(self): | |||||
| """ | |||||
| To find the location of last created object. | |||||
| """ | |||||
| farthest = 0 | |||||
| all_y_locations = [] | |||||
| for obj in bpy.data.objects: | |||||
| all_y_locations.append(obj.location.y) | |||||
| for y in all_y_locations: | |||||
| if y > farthest: | |||||
| farthest = y | |||||
| return farthest | |||||
| def _unlink_from_unwanted_collection(self, collection_name): | |||||
Done Inline Actions_check_for_valid_path() should really only check if the given path is valid. Hiding objects should be the responsibility of another function. zazizizou: `_check_for_valid_path()` should really //only// check if the given path is valid. Hiding… | |||||
| """ | |||||
| Removing the objects from the default Collection. | |||||
| :param collection_name: Name of the user created collection | |||||
| :return: None | |||||
| """ | |||||
| scene = bpy.context.scene | |||||
| # If objects are present in the Master Collection | |||||
| master_collection_list = list(scene.collection.objects) | |||||
| if len(master_collection_list) != 0: | |||||
| for obj in self.list_of_added_objects: | |||||
| scene.collection.objects.unlink(obj) | |||||
| self.list_of_added_objects.remove(obj) | |||||
| # If objects gets added to the default collection - 'Collection' | |||||
| if collection_name != "Collection": | |||||
| while self.list_of_added_objects: | |||||
| for obj in self.list_of_added_objects: | |||||
| try: | |||||
| bpy.data.collections["Collection"].objects.unlink(obj) | |||||
| self.list_of_added_objects.remove(obj) | |||||
| except: | |||||
| pass | |||||
This class does way more than just holding specs. It creates the test object with an offset too.
I think this is too much. Object creation should be the responsibility of GenObject, because only GenObject knows about all the test objects at the same time and therefore can handle the offset between objects much better.