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: added Unity build tasks to BuildTasksContainer #31

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

Conversation

vdenysiuk-sa
Copy link

Purpose of this PR

BuildTaskContainer now contains user's Unity build tasks too.
If user's tasks implement interfaces IPreprocessBuildWithReport, IPostprocessBuildWithReport, IProcessSceneWithReport it will be added to the list

Testing status

  • No tests have been added.

Manual testing status

Tested manually in Unity Editor

@vdenysiuk-sa vdenysiuk-sa self-assigned this Aug 2, 2021
Copy link
Contributor

@stan-osipov stan-osipov left a comment

Choose a reason for hiding this comment

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

Overall looks good, but has to be reformated a bit

namespace StansAssets.Build.Pipeline
{
class DefaultBuildTasksProvider : IBuildTasksProvider
{
public IBuildTasksContainer GetBuildSteps(IUserEditorBuildSettings buildSettings)
public IBuildTasksContainerFull GetBuildSteps(IUserEditorBuildSettings buildSettings)
Copy link
Contributor

Choose a reason for hiding this comment

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

IBuildTasksProvider Should only provide steps for the current build system.
IBuildTasksContainerFull - doesn't make much scene in terms of API.
We want to only display those additional default Unity steps in the editor window. No need to change the tool API.

Comment on lines 9 to 26
public interface IBuildTasksContainerFull : IBuildTasksContainer
{
/// <summary>
/// Pre process build steps. Order matters.
/// </summary>
IReadOnlyList<IOrderedCallback> UnityPreBuildTasks { get; }

/// <summary>
/// Post process build steps. Order matters.
/// </summary>
IReadOnlyList<IOrderedCallback> UnityPostBuildTasks { get; }

/// <summary>
/// Scene process build steps. Order matters.
/// </summary>
IReadOnlyList<IOrderedCallback> UnityProcessSceneTasks { get; }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again we don't really need this to be a part of our API

@aleh-patseyeu
Copy link

This PR depends on the custom branch of the com.stansassets.foundation package.
Currently, in order for to project to compile on the initial clone packages.json should be modified to install the foundation package dependency from the custom branch. These changes are not committed for now.

@aleh-patseyeu aleh-patseyeu linked an issue Jan 18, 2022 that may be closed by this pull request
@aleh-patseyeu
Copy link

aleh-patseyeu commented Jan 20, 2022

Extend the Unity related build steps visualization:

  • add support for CallbackOrderAttribute
  • callback order displaying
  • split list entities to Unity build steps and user-defined build steps handled by our pipeline

I have also

  • removed com.stansassets.foundation custom branch dependency by moving all methods to the project scope.
  • replaced CallbackOrderAttribute usages in core with IOrderedCallback implementations

image

@@ -0,0 +1,7 @@
namespace StansAssets.Build.Pipeline
{
public interface IBuildStepsViewModelProvider
Copy link

@aleh-patseyeu aleh-patseyeu Jan 20, 2022

Choose a reason for hiding this comment

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

I do not like the idea of yet another ISomeProvider and ISomeContainer, especially when the data differs only by type, but I think we should firstly finish with #22 and #23 in order to clarify the usages of providers and containers.

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