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

[Merged by Bors] - gltf: load normal and occlusion as linear textures #1762

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - gltf: load normal and occlusion as linear textures #1762

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
70 changes: 42 additions & 28 deletions crates/bevy_gltf/src/loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use bevy_render::{
pipeline::PrimitiveTopology,
prelude::{Color, Texture},
render_graph::base,
texture::{AddressMode, FilterMode, ImageType, SamplerDescriptor, TextureError},
texture::{AddressMode, FilterMode, ImageType, SamplerDescriptor, TextureError, TextureFormat},
};
use bevy_scene::Scene;
use bevy_transform::{
Expand All @@ -26,7 +26,10 @@ use gltf::{
texture::{MagFilter, MinFilter, WrappingMode},
Material, Primitive,
};
use std::{collections::HashMap, path::Path};
use std::{
collections::{HashMap, HashSet},
path::Path,
};
use thiserror::Error;

use crate::{Gltf, GltfNode};
Expand Down Expand Up @@ -79,12 +82,19 @@ async fn load_gltf<'a, 'b>(

let mut materials = vec![];
let mut named_materials = HashMap::new();
let mut linear_textures = HashSet::new();
for material in gltf.materials() {
let handle = load_material(&material, load_context);
if let Some(name) = material.name() {
named_materials.insert(name.to_string(), handle.clone());
}
materials.push(handle);
if let Some(texture) = material.normal_texture() {
linear_textures.insert(texture.texture().index());
}
if let Some(texture) = material.occlusion_texture() {
linear_textures.insert(texture.texture().index());
}
}

let mut meshes = vec![];
Expand Down Expand Up @@ -191,15 +201,33 @@ async fn load_gltf<'a, 'b>(
.collect();

for gltf_texture in gltf.textures() {
if let gltf::image::Source::View { view, mime_type } = gltf_texture.source().source() {
let start = view.offset() as usize;
let end = (view.offset() + view.length()) as usize;
let buffer = &buffer_data[view.buffer().index()][start..end];
let texture_label = texture_label(&gltf_texture);
let mut texture = Texture::from_buffer(buffer, ImageType::MimeType(mime_type))?;
texture.sampler = texture_sampler(&gltf_texture);
load_context.set_labeled_asset::<Texture>(&texture_label, LoadedAsset::new(texture));
let mut texture = match gltf_texture.source().source() {
gltf::image::Source::View { view, mime_type } => {
let start = view.offset() as usize;
let end = (view.offset() + view.length()) as usize;
let buffer = &buffer_data[view.buffer().index()][start..end];
Texture::from_buffer(buffer, ImageType::MimeType(mime_type))?
}
gltf::image::Source::Uri { uri, mime_type } => {
let parent = load_context.path().parent().unwrap();
let image_path = parent.join(uri);
let bytes = load_context.read_asset_bytes(image_path.clone()).await?;
Texture::from_buffer(
&bytes,
mime_type
.map(|mt| ImageType::MimeType(mt))
.unwrap_or_else(|| {
ImageType::Extension(image_path.extension().unwrap().to_str().unwrap())
}),
)?
}
};
let texture_label = texture_label(&gltf_texture);
texture.sampler = texture_sampler(&gltf_texture);
if linear_textures.contains(&gltf_texture.index()) {
texture.format = TextureFormat::Rgba8Unorm;
}
load_context.set_labeled_asset::<Texture>(&texture_label, LoadedAsset::new(texture));
}

let mut scenes = vec![];
Expand Down Expand Up @@ -252,23 +280,10 @@ async fn load_gltf<'a, 'b>(
fn load_material(material: &Material, load_context: &mut LoadContext) -> Handle<StandardMaterial> {
let material_label = material_label(&material);
let pbr = material.pbr_metallic_roughness();
let mut dependencies = Vec::new();
let texture_handle = if let Some(info) = pbr.base_color_texture() {
match info.texture().source().source() {
gltf::image::Source::View { .. } => {
let label = texture_label(&info.texture());
let path = AssetPath::new_ref(load_context.path(), Some(&label));
Some(load_context.get_handle(path))
}
gltf::image::Source::Uri { uri, .. } => {
let parent = load_context.path().parent().unwrap();
let image_path = parent.join(uri);
let asset_path = AssetPath::new(image_path, None);
Copy link
Contributor

Choose a reason for hiding this comment

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

With this PR no longer using the asset system to load external texture files (and not tracking the image path / not using it for the handle), and not adding them as asset dependencies, I suspect that might mean that the asset system is no longer going to detect if any of the referenced texture files is modified, which means that they will not be hot-reloaded.

(Instead, modifying the GLTF itself would now hot-reload everything, with all the external textures, as they are treated the same as embedded GLB textures.)

Further, we lose the performance/parallelism benefits that we had before, when textures were queued up to be loaded asynchronously by the asset system.

Copy link
Member Author

@mockersf mockersf Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Without a large rework, the asset server is not able to load this kind of assets with external configuration. I think losing the hot-reload of parts of a gltf in exchange for it being correctly loaded is a gain.

Longer term, the Distill integration should provide this feature.

For the async part, I have something working locally (that still need some light work), but would prefer this and #1632 merged first to avoid having too many pr touching the same parts at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the current asset system is kinda limited and quirky, really looking forward to Distill. For now we have to make do with what we have, and I agree that loading and rendering the assets correctly should take priority over the features of the asset system working smoothly.

I am excited about your improvement/fix for async loading, sounds like it should address the performance issues when loading big GLBs with many large textures.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this is the right short term move. I don't think async will solve the performance issues with image loading. We aren't queuing up multiple async tasks at the same time, so while it might run the async task on a separate thread, we won't start the next one until the previous one is finished.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh nobody was claiming the current async stuff was a perf improvement. My bad!

let handle = load_context.get_handle(asset_path.clone());
dependencies.push(asset_path);
Some(handle)
}
}
let label = texture_label(&info.texture());
let path = AssetPath::new_ref(load_context.path(), Some(&label));
Some(load_context.get_handle(path))
} else {
None
};
Expand All @@ -283,8 +298,7 @@ fn load_material(material: &Material, load_context: &mut LoadContext) -> Handle<
metallic: pbr.metallic_factor(),
unlit: material.unlit(),
..Default::default()
})
.with_dependencies(dependencies),
}),
)
}

Expand Down