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.

Sign up for GitHub

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

Jump to bottom

Generate resource graph on bundle #7802

Merged
merged 39 commits into from Oct 25, 2023
Merged

Generate resource graph on bundle #7802

merged 39 commits into from Oct 25, 2023

Conversation

britzl
Copy link
Contributor

@britzl britzl commented Jul 15, 2023 •
edited by AGulev

When building a project using the bob.jar command line tool a resource graph in json format will be saved as build/default/game.graph.json. The resource graph will list all resources included in the game archive, starting at the top level (project root) and following a parent-child hierarchy from the game.project file down through the bootstrap collection to each leaf resource. Example of a graph:

[ {
  "path" : "/<AnonymousRoot>",
  "hexDigest" : null,
  "children" : [ "/main/main.collectionc", "/builtins/render/default.renderc", "/input/game.input_bindingc", "/builtins/input/default.gamepadsc", "/builtins/render/default.display_profilesc", "/builtins/scripts/debugger.luac" ]
}, {
  "path" : "/main/main.collectionc",
  "hexDigest" : "b876bb2748e96cb7ceab9deb9808f534c0b994cf",
  "children" : [ ]
}, {
  "path" : "/builtins/render/default.renderc",
  "hexDigest" : "62fcf66fed9988305fe48996f4d7693b2fac6e49",
  "children" : [ "/builtins/render/default.render_scriptc" ]
}, {
  "path" : "/builtins/render/default.render_scriptc",
  "hexDigest" : "51fe08bdca1d4a79e41bbda87bfa79a151b82c6f",
  "children" : [ ]
}, {
  "path" : "/input/game.input_bindingc",
  "hexDigest" : "9b525bf47b7a7a9c35ad9b07723421404c35dc0f",
  "children" : [ ]
}, {
  "path" : "/builtins/input/default.gamepadsc",
  "hexDigest" : "7c448d18992aefe1c184981f2bb057cf6429321f",
  "children" : [ ]
}]

Fixes #7753

PR checklist

  • Code
    • Add engine and/or editor unit tests.
    • New and changed code follows the overall code style of existing code
    • Add comments where needed
  • Documentation
    • Make sure that API documentation is updated in code comments
    • Make sure that manuals are updated (in github.com/defold/doc)
  • Prepare pull request and affected issue for automatic release notes generator
    • Pull request - Write a message that explains what this pull request does. What was the problem? How was it solved? What are the changes to APIs or the new APIs introduced? This message will be used in the generated release notes. Make sure it is well written and understandable for a user of Defold.
    • Pull request - Write a pull request title that in a sentence summarises what the pull request does. Do not include "Issue-1234 ..." in the title. This text will be used in the generated release notes.
    • Pull request - Link the pull request to the issue(s) it is closing. Use on of the approved closing keywords.
    • Affected issue - Assign the issue to a project. Do not assign the pull request to a project if there is an issue which the pull request closes.
    • Affected issue - Assign the "breaking change" label to the issue if introducing a breaking change.
    • Affected issue - Assign the "skip release notes" is the issue should not be included in the generated release notes.

@britzl britzl marked this pull request as draft July 15, 2023 05:34
@britzl britzl marked this pull request as ready for review August 7, 2023 12:45
@AGulev
Merge dev + fixes (#7989)
d33411c 
* merge

* remove import
@AGulev
Merge branch 'dev' into Issue-7753-generate-resource-graph-on-bundle
35e2b79 
# Conflicts:
#	com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/GameProjectBuilder.java
@AGulev
Copy link
Contributor

AGulev commented Sep 12, 2023

@britzl I added my fix into your branch and resolved the conflict

@britzl britzl requested a review from JCash September 15, 2023 07:55
JCash
JCash requested changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@JCash JCash left a comment

Choose a reason for hiding this comment

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

Looks good.
Some performance improvement possibility when storing the result.
Also some questions and notes.

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/archive/ArchiveBuilder.java Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceNode.java Outdated Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceNode.java Outdated Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceGraphs.java Outdated Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceNode.java Outdated Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/GameProjectBuilder.java Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/GameProjectBuilder.java Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/GameProjectBuilder.java Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/GameProjectBuilder.java Show resolved Hide resolved
JCash
JCash previously approved these changes Sep 24, 2023
View reviewed changes
Copy link
Contributor

@JCash JCash left a comment

Choose a reason for hiding this comment

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

LGTM

Please time it with Family Island to make sure we don't add too much time to it.

AGulev
AGulev reviewed Oct 10, 2023
View reviewed changes
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/Project.java
@@ -467,7 +477,7 @@ public Task<?> createTask(IResource inputResource, Class<? extends Builder<?>> b
builder.setProject(this);
task = builder.create(inputResource);
if (task != null) {
TimeProfiler.addData("output", task.getOutputsString());
TimeProfiler.addData("output", StringUtil.truncate(task.getOutputsString(), 1000));
Copy link
Contributor

Choose a reason for hiding this comment

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

in a big project it thousands of paths, I decided to truncate too long strings

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/archive/ArchiveBuilder.java
@@ -208,13 +181,16 @@ public void write(RandomAccessFile archiveIndex, RandomAccessFile archiveData, P
Collections.sort(entries); // Since it has no hash, it sorts on path

for (int i = entries.size() - 1; i >= 0; --i) {
TimeProfiler.start("Write file");
Copy link
Contributor

Choose a reason for hiding this comment

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

more info about file archiving

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/archive/ArchiveBuilder.java Outdated Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceGraph.java Outdated Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceGraph.java Show resolved Hide resolved
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceNode.java Outdated Show resolved Hide resolved
@AGulev AGulev requested a review from JCash October 10, 2023 15:22
AGulev
AGulev reviewed Oct 13, 2023
View reviewed changes
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/archive/ManifestBuilder.java Outdated
private Set<String> excludedResources = new HashSet<>();
private HashMap<String, ResourceNode> pathToNode = new HashMap<>();
private HashMap<String, List<String>> pathToDependants = new HashMap<>();
private HashMap<String, HashSet<String>> pathToDependants = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Using of a HashSet instead of a List helps to make sure that all the resource entries are unique

AGulev
AGulev reviewed Oct 16, 2023
View reviewed changes
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceGraph.java
}
flagAllNonExcludedResources(child, visited);
}
}
Copy link
Contributor

@AGulev AGulev Oct 16, 2023 •
edited

Choose a reason for hiding this comment

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

Visit all the nodes in non-excluded collections once and flag them as used in the main bundle. We can exclude everything else.

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/archive/ManifestBuilder.java
}

public List<String> getDependants(String filepath) throws IOException {
public HashSet<ResourceNode> getAllDependants(ResourceNode node) throws IOException {
Copy link
Contributor

@AGulev AGulev Oct 16, 2023 •
edited

Choose a reason for hiding this comment

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

it's easier to operate with nodes instead of strings here

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/archive/ManifestBuilder.java
if (!child.relativeFilepath.endsWith("collectionproxyc")) {
dependants.addAll(getDependants(child.relativeFilepath));
if (!child.checkType(ResourceNode.Type.CollectionProxy)) {
dependants.addAll(getAllDependants(child));
Copy link
Contributor

@AGulev AGulev Oct 16, 2023 •
edited

Choose a reason for hiding this comment

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

I avoid operations over strings as much as possible. I use already known info about the node (as type e.g. collection proxy)

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/archive/ManifestBuilder.java Outdated
HashSet<ResourceNode> allProxyDependants = this.getAllDependants(proxyNode);
for (ResourceNode dependant : allProxyDependants) {
// Exclude resources referenced from the main bundle
if (dependant.isInMainBundle()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Exclude resources we use at least once in the main bundle

AGulev
AGulev reviewed Oct 17, 2023
View reviewed changes
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceNode.java
public boolean checkType(Type type) {
// ExcludedCollectionProxy is CollectionProxy but CollectionProxy isn't ExcludedCollectionProxy
if (this.nodeType == Type.ExcludedCollectionProxy) {
return type == Type.CollectionProxy || type == Type.ExcludedCollectionProxy;
Copy link
Contributor

Choose a reason for hiding this comment

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

Old approach with excludedflag was confusing.
Semantically the flag was about resource (flag on a collection proxy), not a ResourceNode, so I moved it into types to make sure it's clear now.

Also, we have inMainBundle that helps to figure out if resource excluded or not.

com.dynamo.cr/com.dynamo.cr.bob.test/src/com/dynamo/bob/archive/test/ManifestTest.java
@@ -460,90 +489,9 @@ public void testCreateManifest_Resources() throws NoSuchAlgorithmException, Inva
}

if (current.getUrl().equals("/main/level1.collectionproxyc")) {
assertEquals(5, current.getDependantsCount());
printDeps(data, current);
assertEquals(4, current.getDependantsCount());
Copy link
Contributor

Choose a reason for hiding this comment

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

Old code worked wrong:
root

+--/main/main.collectionc
   +--/main/main.goc
   |  +--/main/main.scriptc
   |
   +--/main/shared_go.goc
   |
   +--/main/dynamic.collectionc
   |  +--/main/dynamic.goc
   |
   +--/main/level1.collectionproxyc
      +--/main/level1.collectionc
         +--/main/dynamic.goc
         +--/main/level1.goc
         |  +--/main/level1.scriptc
         |  +--/main/shared_go.goc
         |
         +--/main/level2.collectionproxyc
            +--/main/level2.collectionc
                +--/main/dynamic.goc
                +--/main/level2.goc
                +--/main/level2.soundc

new code includes only:

    [junit] /main/level1.goc
    [junit] /main/level1.scriptc
    [junit] /main/level2.collectionproxyc
    [junit] /main/level1.collectionc

old code includes:

    [junit] /main/level1.goc
    [junit] /main/level1.scriptc
    [junit] /main/shared_go.goc
    [junit] /main/level2.collectionproxyc
    [junit] /main/level1.collectionc

But /main/shared_go.goc used in the main bundle and shouldn't be included.

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/GameProjectBuilder.java Outdated Show resolved Hide resolved
AGulev
AGulev reviewed Oct 18, 2023
View reviewed changes
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/graph/ResourceGraph.java
if (shouldPublishLU) {
generator.writeFieldName("isInMainBundle");
generator.writeBoolean(node.isInMainBundle());
}
Copy link
Contributor

@AGulev AGulev Oct 18, 2023 •
edited

Choose a reason for hiding this comment

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

@britzl isInMainBundle isn't useful if a user doesn't use LiveUpdate, and it may mislead a developer. So, I don't include this data if LU is off.

JCash
JCash approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@JCash JCash left a comment

Choose a reason for hiding this comment

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

LGTM

@AGulev AGulev merged commit aa9da3a into dev Oct 25, 2023
22 checks passed
@AGulev AGulev deleted the Issue-7753-generate-resource-graph-on-bundle branch October 25, 2023 14:40
ultimanidev pushed a commit to ultimanidev/defold that referenced this pull request Nov 8, 2023
@britzl @AGulev @ultimanidev
Generate resource graph on bundle (defold#7802)
1d9045e 
* Initial commit

* Update ResourceNodeGraph.java

* Update ResourceNodeGraph.java

* Fix imports for tests

* Improved tostring

* Forgot to call visitor.leave() on early out

* Split resource and graph builds

* Merged set and graph creation

* Use resources instead of resource paths

* Cache hex digests

* Added hex digest to resource nodes

* Added hashCode and equals to resource nodes

* Set hex digests on all nodes in a resource graph

* Changed how the archive builder is created

* Fix keep-unused

* Set excluded resources

* Merge dev + fixes (defold#7989)

* merge

* remove import

* Change from absolute to relative paths

* Use relative paths in resource nodes

* Added timers

* Added option to write graph directly to stream

* Reworked the resource graph

* Fixed tests

* Moved test

* take into account only unique children

* fix issue with ManifestBuilder

* fix issue with resurce exclusion

* fix an issue in tests

* semantics refactoring

* remove old comment

* do not include information about the main bundle if user doesn't use LU

---------

Co-authored-by: Alexey Gulev <me@agulev.com>
@AGulev AGulev mentioned this pull request Jan 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AGulev AGulev AGulev left review comments

@JCash JCash JCash approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Information about full dependency graph
3 participants
@britzl @AGulev @JCash