o3de / sig-testing

Documentation and materials for the Open 3D Engine Test Special Interest Group
9 stars 7 forks source link

Proposed RFC Suggestion: Moving `editor_entity_utils` to the `EditorPythonBindings` gem #67

Open cgalvan opened 1 year ago

cgalvan commented 1 year ago

Summary:

I am proposing moving the editor_entity_utils python module out of the EditorPythonTestTools package, so that it can be used outside of its current project (AutomatedTesting). The proposed new location is the EditorPythonBindings gem.

What is the motivation for this suggestion?

This is important because currently, the editor_entity_utils module can only be used in the AutomatedTesting project. This is preventing users from leveraging this module in their own projects that are generally useful beyond the scope of just automated tests (e.g. helper methods for creating entities, managing components on those entities, modifying properties on components, etc...).

This is actually a continuation of a previous change made (https://github.com/o3de/sig-content/issues/61) where we moved the pyside_utils module from the AutomatedTesting project to the QtForPython gem for the same motivation of using the module in any user project. There are other modules in EditorPythonTestTools that would be generally useful to move as well, but it will be easier to move specific modules at a time.

Making these modules more generally accessible has been raised several times in the past, most recently as a result of the following PR for the URDF Exporter Gem (https://github.com/o3de/o3de-technicalart/pull/5). In this case, the POC had to replicate a lot of the functionality that is provided by the editor_entity_utils because it couldn't be used outside of the AutomatedTesting project.

Suggestion design description:

What are the advantages of the suggestion?

What are the disadvantages of the suggestion?

Future considerations

As mentioned earlier, there are additional modules in EditorPythonTestTools that would be strong candidates to be moved outside of the AutomatedTesting project as well so that they could be used in any project. It might be beneficial to identify those as part of this work, even if we choose to only move the editor_entity_utils module right now.

Also, once the editor_entity_utils module has been moved, we should work with @shawstar to update the URDF Exporter gem (https://github.com/o3de/o3de-technicalart/pull/5) as an initial customer/use-case to replace its current helper methods for entity/component/property actions with corresponding editor_entity_utils APIs.

cgalvan commented 1 year ago

@jckand-amzn @Kadino

HogJonny-AMZN commented 1 year ago

+1

benblack75 commented 1 year ago

This would directly be a benefit to work we are doing with the DCCsi

tkothadev commented 1 year ago

+1, I would like for the python editor tooling to get more tools

AMZN-Dk commented 1 year ago

I would support this RFC being written.

Kadino commented 1 year ago

I would support this RFC being written.

To clarify this comment: In SIG-Testing we use this issue template for the start of an RFC or to request that someone in SIG-Testing get assigned to define an RFC. That owner then uses the Pull Request process to iterate on the text of the RFC, seeking approval to merge and accept the RFC.

Kadino commented 1 year ago

This would not add any new tools, but it may make what exists slightly more discoverable. Given that it is a Python script, it is equally accessible from inside the AutomatedTesting folders as it would be in the EPB gem. At least unless customers are deleting files from their repo.

I'm not quite sure that the entire file is suitable to directly move into the EditorPythonBindings gem. I believe it currently expects various dependencies in the AutomatedTesting project are enabled, not necessarily just the dependencies of the EPB gem. It may be surprising if certain functionality is unavailable at runtime, though that may be mitigated by adding error messages. It also currently depends on other scripts in its same directory. Like many automation tools, it's accumulated a bit of a "big ball of mud" design antipattern.

I would definitely approve of using the existing file as inspiration for new, high-quality tools in an easily discoverable location. And while such functions are relevant to test-writing, it may be more appropriate for SIG-Content to continue to own the interface and productivity tools for EditorPythonBindings.

Kadino commented 1 year ago

From RFC discussion: good to first reduce cross-dependencies in these files. This would simplify moving this file and updating all dependencies in-place, such that it doesn't need a longer deprecation pattern.

Kadino commented 1 year ago

Discoverability is still an issue, even if it living in this Gem. Can address this by adding Documentation which points to examples in AutomatedTesting and the URDF Exporter Gem. Gem JSON description may also be useful to update.

Kadino commented 1 year ago

Additional files such as asset_utils could be appropriate to live in the same location, if they can't immediately move it would be good to track issues stating why.

smurly commented 1 year ago

disable_component function in EditorComponent class is marked for deprecation and has been for some time. Do we want to finish deprecation and remove this during the transition