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

Feat: Copy several files to destination directory (optionally) #34

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MoxieWhimsy
Copy link

This changes the CopyFileToDirectory BuildStep to a CopyFilesToDirectory BuildStep. The OnValidate method is included to copy the m_filePath property from existing CopyFileToDirectory scriptable objects into the m_filePaths property.

Copy link
Collaborator

@Rinal Rinal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!
Please, do not remove the previous step Editor/IO/Files/CopyFileToDirectory.cs, as it can break already created pipelines. Create a new step

{
[SerializeField] private PathProperty[] m_filePaths = default;
[System.Obsolete, HideInInspector]
[SerializeField] private PathProperty m_filePath = default;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like can be removed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Since we won't be replacing CopyFileToDirectory, we don't need to replicate its properties.

)]
public sealed class CopyFilesToDirectory : BuildStep
{
[SerializeField] private PathProperty[] m_filePaths = default;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can name it m_sourceDirectory

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I will add a property for source directory. It may simplify using this build step. However I will also rename this to m_sourceFiles for clarity.

Editor/IO/Files/CopyFilesToDirectory.cs Outdated Show resolved Hide resolved
[SerializeField] private PathProperty m_filePath = default;
[SerializeField] private PathProperty m_destination = default;

public override async Task Execute()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check is the m_sourceDirectory path is a directory path. If not - throw an exception

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I will make sure m_destinationDirectory is a directory path.

@Rinal Rinal self-assigned this May 15, 2023
@MoxieWhimsy
Copy link
Author

OK. Looking back at this commit, I overcomplicated adding this BuildStep and that led to other extra lines and methods.

It looks like I deliberately gave the new step the same meta file and guid as the previous CopyFileToDirectory because (at least at the time) Unity used the guid to identify ScriptableObject instances. I'm not sure if this step would have been sufficient to prevent old piplelines from breaking. I likely tested it only on my own pipleline.

I'll refactor to not remove the existing step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants