Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Allow hot reload without syncing all asset files to devFS #21436

Merged
jonahwilliams merged 13 commits into from
Oct 2, 2020
Merged

Allow hot reload without syncing all asset files to devFS #21436

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d870dea
Allow hot reload without syncing all asset files to devFS
jonahwilliams Sep 26, 2020
847b53d
Update asset_manager.cc
Sep 26, 2020
62cea76
only preserve original asset manager
jonahwilliams Sep 27, 2020
ec3ba29
Update asset_manager.cc
Sep 27, 2020
9ebdec9
Update engine.cc
Sep 28, 2020
262912a
always re-create original asset dir
jonahwilliams Sep 30, 2020
0ebce10
Merge branch 'double_devfs' of github.com:jonahwilliams/engine into d…
jonahwilliams Sep 30, 2020
5a95412
remove missed merge
jonahwilliams Sep 30, 2020
cb3e7b1
fix order of asset resolvers
jonahwilliams Sep 30, 2020
32f85bd
add shell unit test for setting asset dir path
jonahwilliams Oct 1, 2020
f42dd5e
maybe this works
jonahwilliams Oct 2, 2020
f004337
make a private member
jonahwilliams Oct 2, 2020
ee6772a
format
jonahwilliams Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions assets/asset_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ void AssetManager::PushBack(std::unique_ptr<AssetResolver> resolver) {
resolvers_.push_back(std::move(resolver));
}

void AssetManager::Merge(std::shared_ptr<AssetManager> assetManager) {
Copy link
Member

@zanderso zanderso Sep 29, 2020

Choose a reason for hiding this comment

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

About the name, Merge typically means that all the elements will be combined somehow, but it doesn't look like that's what this is doing.

I'd like to suggest a better name, but I think I don't understand what is special about the last resolver of assetManager.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Sep 29, 2020

Choose a reason for hiding this comment

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

Maybe PreserveBundleManager or something like that?

The very last resolve is the first one ever added, which is the one that resolves assets from the APK/IPA bundle that is installed with the device. Preserving it means that we don't need to sync every asset into the devFS when connecting to a device

Copy link
Member

@jason-simmons jason-simmons Sep 29, 2020

Choose a reason for hiding this comment

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

Would something like this work?

  • AssetManager adds an API to declare which asset resolvers should be preserved across a hot reload
  • Shell::OnServiceProtocolSetAssetBundlePath retains those resolvers when constructing the new AssetManager

Copy link
Contributor Author

@jonahwilliams jonahwilliams Sep 29, 2020

Choose a reason for hiding this comment

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

I think it would have to be done in engine.cc since the current asset manager will be private. I could add a param to the directory asset resolver that controls whether it should be preserved and then set this to true for the initial resolvers.

if (assetManager == nullptr) {
return;
}
if (assetManager->resolvers_.size() > 0) {
resolvers_.push_back(std::move(
assetManager->resolvers_[assetManager->resolvers_.size() - 1]));
}
}

// |AssetResolver|
std::unique_ptr<fml::Mapping> AssetManager::GetAsMapping(
const std::string& asset_name) const {
Expand Down
2 changes: 2 additions & 0 deletions assets/asset_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class AssetManager final : public AssetResolver {

void PushBack(std::unique_ptr<AssetResolver> resolver);

void Merge(std::shared_ptr<AssetManager> assetManager);
Copy link
Member

@zanderso zanderso Sep 29, 2020

Choose a reason for hiding this comment

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

Since this is doing something fancier than a straight-up merge as I think is usually understood, it would be good to have a bunch of comments here about what this does.


// |AssetResolver|
bool IsValid() const override;

Expand Down
3 changes: 2 additions & 1 deletion shell/common/engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,13 @@ bool Engine::UpdateAssetManager(
if (asset_manager_ == new_asset_manager) {
return false;
}

auto old_asset_manager = asset_manager_;
asset_manager_ = new_asset_manager;

if (!asset_manager_) {
return false;
}
asset_manager_->Merge(old_asset_manager);
Copy link
Member

@zanderso zanderso Sep 29, 2020

Choose a reason for hiding this comment

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

More comments please =)


// Using libTXT as the text engine.
font_collection_.RegisterFonts(asset_manager_);
Expand Down