Closed javagl closed 7 months ago
Even if this is fixed, I think that the implementation of
applyKHRTextureTransform::applyKHRTextureTransform
itself is broken: It "bakes" the texture transform that is defined in the texture into the texture coordinates of the mesh primitive.
I agree. I don't think the texture transform should be baked into the texcoords since it's defined in the texture, not in the mesh. Maybe instead TexCoordFromAccessor
should be given the texture transform and transform coordinates on the fly.
I'll have to cross-check this with the linked file/PR.
There are several layers of considerations, revolving around where and how these transformed texture coordinates should be obtained and stored.
On the lowest level, one might naïvely wish for some pseudocode like
AccessorView transformed = apply(original, offset, rotation, scale);
This could, in fact, be implemented as a real view (without any additional memory overhead), if AccessorView
was an interface (i.e. a purely virtual class). But with the current state, another layer of "decoupling" would be necessary (AccessorViewView
? 😬 ). Also, right now, it's not really possible to ~"create an instance of an AccessorView
". It does not own the internal const std::byte* _pData
, so there's nobody who can delete
that, eventually.
<edit>
@lilleyse From a quick look at the linked PR, this part of TexCoordsFromAccessor
could indeed be the right place to perform such transforms. Still with a few options for the details, but conceptually, that could be a convenient solution.
</edit>
On the highest level, the interdependency to KHR_materials_variants
raises some questions. Someone may "process" a MeshPrimitive
in some way, read the texture coordinates, and send them to the renderer. But then, someone switches to another material variant, and ... suddenly, the texture coordinates have to be updated/changed. Some thought will have to be put into the right structures for maintaining this on the runtime engine side...
Below I drafted a few tests to better see the interplay of the different pieces of information. The relevant functions are
obtainMaterial
: Returns the Material
for a MeshPrimitive
, depending on the selected material variant
Material
, one can obtain the desired TextureInfo
(base color, metal-roughness...)obtainTransformedTextureCoordinates
: For a given MeshPrimitive
+TextureInfo
, this returns the texture coordinates. If the texture defines a KHR_texture_transform
, then they are transformed accordingly
The printTextureCoordinateInfos
is the "entry point" that just iterates over all mesh primitives and their material (for a given variant name), and prints the (possibly transformed) texture coordinates.
This is really just a DRAFT. How this will be solved in cesium-native
(and which parts of that will be in the responsibility of the runtime engine, which handles the material variants) is still completely open.
#include "Common/Common.h"
#include "GltfTests/GltfTests.h"
#include <CesiumGltf/AccessorView.h>
#include <CesiumGltf/ExtensionKhrTextureTransform.h>
#include <CesiumGltf/ExtensionMeshPrimitiveKhrMaterialsVariants.h>
#include <CesiumGltf/ExtensionModelKhrMaterialsVariants.h>
#include <CesiumGltf/Model.h>
#include <CesiumGltfReader/GltfReader.h>
#include <iostream>
#include <string>
template <typename C, typename T>
bool contains(const C &container, const T &value) {
return std::find(container.begin(), container.end(), value) !=
container.end();
}
// Obtain the material of the given mesh primitive that it
// has when the given material variant is activated
const CesiumGltf::Material *
obtainMaterial(const CesiumGltf::Model &model,
const CesiumGltf::MeshPrimitive &primitive,
std::optional<std::string> optionalMaterialsVariantsName) {
// When the material variants name is empty, just return
// the default material of the primitive
if (!optionalMaterialsVariantsName.has_value()) {
return CesiumGltf::Model::getSafe(&model.materials, primitive.material);
}
const std::string materialsVariantsName =
optionalMaterialsVariantsName.value();
// Check if the model defines materials variants,
// and return the default material if it does not
bool materialVariantsIsPresentOnModel =
model.hasExtension<CesiumGltf::ExtensionModelKhrMaterialsVariants>();
if (!materialVariantsIsPresentOnModel) {
SPDLOG_INFO("No material variants on model");
return CesiumGltf::Model::getSafe(&model.materials, primitive.material);
}
// Compute the index of the materials variants name in the model extension,
// and return the default material if it is invalid
const CesiumGltf::ExtensionModelKhrMaterialsVariants *materialVariantsGltf =
model.getExtension<CesiumGltf::ExtensionModelKhrMaterialsVariants>();
const std::vector<CesiumGltf::ExtensionModelKhrMaterialsVariantsValue>
variants = materialVariantsGltf->variants;
int32_t variantIndex = -1;
for (size_t i = 0; i < variants.size(); i++) {
const CesiumGltf::ExtensionModelKhrMaterialsVariantsValue &variant =
variants[i];
if (variant.name == materialsVariantsName) {
variantIndex = static_cast<int32_t>(i);
break;
}
}
if (variantIndex == -1) {
SPDLOG_INFO("Variant name {} not found in variants", materialsVariantsName);
return CesiumGltf::Model::getSafe(&model.materials, primitive.material);
}
// Check if the mesh primitive defines materials variants,
// and return the default material if it does not
bool materialVariantsIsPresentOnMeshPrimitive = primitive.hasExtension<
CesiumGltf::ExtensionMeshPrimitiveKhrMaterialsVariants>();
if (!materialVariantsIsPresentOnMeshPrimitive) {
SPDLOG_INFO("No material variants on mesh primitive");
return CesiumGltf::Model::getSafe(&model.materials, primitive.material);
}
// Search through the mappings that are defined in the
// mesh primitive, and find the material that corresponds
// to the actvie variant
const CesiumGltf::ExtensionMeshPrimitiveKhrMaterialsVariants
*materialVariants = primitive.getExtension<
CesiumGltf::ExtensionMeshPrimitiveKhrMaterialsVariants>();
const std::vector<
CesiumGltf::ExtensionMeshPrimitiveKhrMaterialsVariantsMappingsValue>
mappings = materialVariants->mappings;
for (size_t i = 0; i < mappings.size(); i++) {
const CesiumGltf::ExtensionMeshPrimitiveKhrMaterialsVariantsMappingsValue
&mapping = mappings[i];
const bool isActive = contains(mapping.variants, variantIndex);
if (isActive) {
return CesiumGltf::Model::getSafe(&model.materials, mapping.material);
}
}
SPDLOG_INFO("Variant {} not found in primitive", variantIndex);
return CesiumGltf::Model::getSafe(&model.materials, primitive.material);
}
// From cesium-native\CesiumGltfReader\src\applyKHRTextureTransform.cpp
void transformAccessorData(
const CesiumGltf::AccessorView<glm::vec2> &accessorView,
std::vector<std::byte> &data,
const CesiumGltf::ExtensionKhrTextureTransform &textureTransform) {
if (textureTransform.offset.size() < 2 || textureTransform.scale.size() < 2) {
SPDLOG_INFO("Invalid offset or scale in texture transform");
return;
}
float Rotation = static_cast<float>(textureTransform.rotation);
if (Rotation == 0.0f) {
float OffsetX = static_cast<float>(textureTransform.offset[0]);
float OffsetY = static_cast<float>(textureTransform.offset[1]);
float ScaleX = static_cast<float>(textureTransform.scale[0]);
float ScaleY = static_cast<float>(textureTransform.scale[1]);
glm::vec2 *uvs = reinterpret_cast<glm::vec2 *>(data.data());
for (int i = 0; i < accessorView.size(); i++) {
glm::vec2 uv = accessorView[i];
// std::cout << "old " << i << " is " << uv.x << " " << uv.y << std::endl;
uv.x = uv.x * ScaleX + OffsetX;
uv.y = uv.y * ScaleY + OffsetY;
// std::cout << "new " << i << " is " << uv.x << " " << uv.y << std::endl;
*uvs++ = uv;
}
} else {
glm::vec2 Offset(textureTransform.offset[0], textureTransform.offset[1]);
glm::vec2 Scale(textureTransform.scale[0], textureTransform.scale[1]);
glm::mat3 translation = glm::mat3(1, 0, 0, 0, 1, 0, Offset.x, Offset.y, 1);
glm::mat3 rotation = glm::mat3(cos(Rotation), sin(Rotation), 0,
-sin(Rotation), cos(Rotation), 0, 0, 0, 1);
glm::mat3 scale = glm::mat3(Scale.x, 0, 0, 0, Scale.y, 0, 0, 0, 1);
glm::mat3 matrix = translation * rotation * scale;
glm::vec2 *uvs = reinterpret_cast<glm::vec2 *>(data.data());
for (int i = 0; i < accessorView.size(); i++) {
*uvs++ = glm::vec2((matrix * glm::vec3(accessorView[i], 1)));
}
}
}
// Obtain a view on the accessor data for the `TEXCOORD_<texCoord>`
// attribute of the given primitive (or nullopt if something does
// not add up...)
std::optional<CesiumGltf::AccessorView<glm::vec2>>
obtainTextureCoordinatesAccessorView(const CesiumGltf::Model &model,
const CesiumGltf::MeshPrimitive &primitive,
int64_t texCoord) {
auto attributeIt =
primitive.attributes.find("TEXCOORD_" + std::to_string(texCoord));
if (attributeIt == primitive.attributes.end()) {
SPDLOG_INFO("No TEXCOORD_{} found in mesh primitive", texCoord);
return std::nullopt;
}
const int32_t accessorIndex = attributeIt->second;
const CesiumGltf::Accessor *accessor =
CesiumGltf::Model::getSafe(&model.accessors, attributeIt->second);
if (!accessor) {
SPDLOG_INFO("Accessor {} not found", accessorIndex);
return std::nullopt;
}
const CesiumGltf::AccessorView<glm::vec2> accessorView(model, *accessor);
if (accessorView.status() != CesiumGltf::AccessorViewStatus::Valid) {
SPDLOG_INFO("Accessor {} not valid", accessorIndex);
return std::nullopt;
}
return accessorView;
}
// Inefficiently extract the data from the given accessor view
// into a vector of 2D vectors
std::vector<glm::vec2>
extractData(CesiumGltf::AccessorView<glm::vec2> accessorView) {
int64_t size = accessorView.size();
std::vector<glm::vec2> data;
for (int64_t i = 0; i < size; i++) {
const glm::vec2 &element = accessorView[i];
data.push_back(element);
}
return data;
}
// Obtain the texture coordinates of the given mesh primitive
// for the given texture info. If the given texture info
// defines a KHR_texture_transform, then the coordinates
// are transformed accordingly
std::optional<std::vector<glm::vec2>> obtainTransformedTextureCoordinates(
const CesiumGltf::Model &model, const CesiumGltf::MeshPrimitive &primitive,
const CesiumGltf::TextureInfo &textureInfo) {
// If there is no texture transform extension, then
// just return the original accessor view data
const CesiumGltf::ExtensionKhrTextureTransform *pTextureTransform =
textureInfo.getExtension<CesiumGltf::ExtensionKhrTextureTransform>();
if (!pTextureTransform) {
int64_t texCoord = textureInfo.texCoord;
std::optional<CesiumGltf::AccessorView<glm::vec2>> optionalAccessorView =
obtainTextureCoordinatesAccessorView(model, primitive, texCoord);
if (!optionalAccessorView) {
return std::nullopt;
}
return extractData(optionalAccessorView.value());
}
// Obtain the accessor view for the texture coordinates
// that are defined in the texture transform extension
int64_t texCoord = textureInfo.texCoord;
if (pTextureTransform->texCoord) {
texCoord = *pTextureTransform->texCoord;
}
std::optional<CesiumGltf::AccessorView<glm::vec2>>
optionalTextureCoordinatesAccessorView =
obtainTextureCoordinatesAccessorView(model, primitive, texCoord);
if (!optionalTextureCoordinatesAccessorView.has_value()) {
return std::nullopt;
}
// Transform the accessor view data (using the function
// copied from `cesium-native`, just as a workaround here)
CesiumGltf::AccessorView<glm::vec2> textureCoordinatesAccessorView =
optionalTextureCoordinatesAccessorView.value();
const int64_t count = textureCoordinatesAccessorView.size();
const int64_t stride = 2 * 4;
const int64_t offset = 0;
const int64_t size = count * stride;
std::vector<std::byte> data;
data.resize(size);
transformAccessorData(textureCoordinatesAccessorView, data,
*pTextureTransform);
CesiumGltf::AccessorView<glm::vec2> transformedAccessorView(
data.data(), stride, offset, count);
return extractData(transformedAccessorView);
}
void printTextureCoordinateInfos(CesiumGltf::Model &model,
std::optional<std::string> variantName) {
const std::vector<CesiumGltf::Mesh> meshes = model.meshes;
for (size_t m = 0; m < meshes.size(); m++) {
const CesiumGltf::Mesh &mesh = meshes[m];
const std::vector<CesiumGltf::MeshPrimitive> meshPrimitives =
mesh.primitives;
for (size_t p = 0; p < meshPrimitives.size(); p++) {
const CesiumGltf::MeshPrimitive &meshPrimitive = meshPrimitives[p];
SPDLOG_INFO("Texture coordinate infos for mesh primitive {} of mesh {}, "
"variant {}",
p, m, variantName.value_or("(none)"));
const CesiumGltf::Material *material =
obtainMaterial(model, meshPrimitive, variantName);
if (!material) {
SPDLOG_INFO("No material in primitive");
return;
}
std::optional<CesiumGltf::TextureInfo> optionalTextureInfo =
material->pbrMetallicRoughness->baseColorTexture;
std::optional<std::vector<glm::vec2>> optionalTextureCoordinates =
obtainTransformedTextureCoordinates(model, meshPrimitive,
optionalTextureInfo.value());
if (!optionalTextureCoordinates.has_value()) {
SPDLOG_INFO("Could not obtain transformed texture coordinates");
return;
}
std::vector<glm::vec2> textureCoordinates =
optionalTextureCoordinates.value();
const size_t limit = 10;
size_t n = std::min(limit, textureCoordinates.size());
for (size_t i = 0; i < n; i++) {
const glm::vec2 &texCoord = textureCoordinates[i];
SPDLOG_INFO("At {} have {}, {}", i, texCoord.x, texCoord.y);
}
if (textureCoordinates.size() > limit) {
SPDLOG_INFO("...");
}
}
}
}
void testGltfTextureTransformWorkaround(const std::string &basePath,
const std::string &fileName,
const std::string &exampleVariantName) {
CesiumCpp::GltfTests::DataReader dataReader =
CesiumCpp::GltfTests::createFileReader(basePath);
std::vector<std::byte> data = dataReader(fileName);
const std::byte *dataBegin = reinterpret_cast<const std::byte *>(data.data());
CesiumGltfReader::GltfReader gltfReader;
CesiumGltfReader::GltfReaderOptions options;
// Set this to 'false'!
options.applyTextureTransform = false;
CesiumGltfReader::GltfReaderResult gltfReaderResult =
gltfReader.readGltf(gsl::span(dataBegin, data.size()), options);
bool success = gltfReaderResult.model.has_value();
if (!gltfReaderResult.model.has_value()) {
SPDLOG_INFO("Could not load model");
return;
}
CesiumGltf::Model modelResult = gltfReaderResult.model.value();
printTextureCoordinateInfos(modelResult, std::nullopt);
printTextureCoordinateInfos(modelResult, exampleVariantName);
}
int main(int argc, char **argv) {
const std::string basePath = "C:/Develop/gltfGen/gltfGen/";
//const std::string subPath = "textureTransformTest.glb";
const std::string subPath = "textureTransformTestTwoTextures.glb";
const std::string exampleVariantName = "exampleVariant";
testGltfTextureTransformWorkaround(basePath, subPath, exampleVariantName);
return 0;
}
Hello,
In my code, I am creating a unique texture accessor for the transformed texture coordinates, but I am reusing the TextureInfo and the texture coordinate set. Could this be causing the double transform issue, where the texture coordinates are transformed twice? Would creating a new TextureInfo and a new primitive attribute fix this issue?
@joseph-kaile For now, the main point is that a glTF that contains KHR_texture_transform
may cause several issues. The details depend on the structure of the input glTF, and only apply when loading it with the default of gltfReaderOptions.applyTextureTransform = true
instead of explicitly setting this to false
.
Regarding your question ... I might have to dive deeper into some implementation details of cesium-native
, or ask details about your code, and maybe someone from the cesium-native
core team wants to chime in here.
It sounds like you are already doing the texture transform manually (i.e. you already set that applyTextureTransform
flag to false
)?
In this case, there are different options to solve that (and my workaround draft was mainly an attempt to wrap my head around that...),
I am creating a unique texture accessor for the transformed texture coordinates, but I am reusing the TextureInfo and the texture coordinate set. Could this be causing the double transform issue, where the texture coordinates are transformed twice?
The TextureInfo
is usually not modified. And when you are using the original AccessorView
that was obtained for the TEXCOORD_n
, then there is no way to (directly) modify that data either (except for drilling into the underlying buffer). So if you have something like (pseudocode)
AccessorView<vec2> texCoords = meshPrimitive->attributes["TEXCOORD_n"];
TextureInfo textureInfo = material->pbrMetallicRoughness->baseColorTexture;
glm::mat3 transform = textureInfo->magicallyGetExtension("ExtensionKhrTextureTransform")->createMatrix();
for (i...) {
vec2 transformedTexCoord = texCoords[i] * transform;
...
}
then there should be no place where the transform could be applied twice.
(What makes this a bit complicated is that the KHR_texture_transform
extension can override the texCoord
of the original TextureInfo
- so the transformed coordinates depend on which material is used, and which texture this is about, and whether this particular texture has the extension, and whether this one overrides the texCoord
that is used for accessing the mesh primitive attribute 😵💫 )
Thank you everyone for looking into this issue. I am no longer blocked after manually transforming the uvs. When reading the KHR_texture_transform it seems like the proper usecase is to do the uv transformations in a shader. I guess the question is should cesium-native even worry about it? Just stop doing the transforms all together in cesium-native and let the client/game-engine pass the offset and scale to the shader. Thanks again!
@ZackOfAllTrades Indeed, applying the texture transform in the shader is probably the preferred solution (also in terms of memory requirements and performance). But obtaining the right information that has to be passed to the shader can still be a bit cumbersome, and cesium-native
could try to support the clients here.
The question is: How?
When you look at the code that you had to write in your implementation, then there's probably some chunk of code that will be very similar for every client, and where you might have thought: "Meh, why can't cesium-native
offer that shaderData = Magic::giveMeThatData()
function here?"
As I said in the 'disclaimer' above that 'draft solution', I don't have a clear idea how clients are handling these things, particularly when it comes to combining this with KHR_material_variants
. One has to keep in mind...
texCoord
(i.e. the TEXCOORD_n
attribute) that has to be used...I could imagine that there might be some helper functions in cesium-native
, but I'm not sure what they could receive/return exactly. In the most simple case, one part could be to just offer the transform information from the extension as a glm::mat3
(unless people want to use optmized shader implementations when the rotation
value in the extension is 0
....). But beyond that, I'll have to look at the existing engines (Cesium for Unreal/Unity/Omniverse...) to get a better idea.
(Eventually, there might still be some functionality that is similar to the 'draft solution', as a kind of simplification/fallback for doing all this on the CPU, for cases where the custom shader approach is not applicable)
@javagl, I'm thinking of it in terms of the cesium terrain with raster overlays where we have already trained the client that there is an offset and scale, inside the RasterMappedTo3DTile its called getTranslation() and getScale().
We expect the client to get this this per image translation and scale when switching overlays. I was suggesting we take the same approach here where we expect the client to handle a per texture transformation when switching textures.
When switching materials you will need to forward the information from the CesiumGltf::TextureInfo to the client so you can swap the uvs and texture for the primitive, at this time they can also change the offset and scale.
We could make it easier and the CesiumGltf::TextureInfo could always have the transformation and cesium-native could fill it in behind the scenes whether it be identity or a KHR transformation (example does not show rotation).
For some details, the main cesium-native
developers will have to chime in here - some vaguess and assumptions ahead:
I imagine that clients mainly use the TextureInfo
when they are translating the glTF structures into their rendering structures. Where an how they are managing these structures (in view of the association between primitive and material, and material and textureInfos) is probably very specific for the client.
Nevertheless, the TextureInfo
might appear to be a place where one could conveniently add the offset/scale. But ... the TextureInfo
is auto-generated, and can not (easily) receive members that are not part of the schema. Additionally, clients might want to be able to differentiate between three cases:
(The shader complexity will increase along these cases, and ... people tend to be very sensitive to that...)
I could imagine something like plain
struct TextureTransform {
glm::vec2 offset;
double rotation;
glm::vec2 scale;
}
that is independent of the TextureInfo
struct itself, but can be obtained conveniently - maybe with some utility function
TextureTransform t = Magic.get(model, textureInfo);
(with some std::optional
s sprinkled in, to differentiate the cases that may be relevant for the client)
@j9liu is this fixed by using TextureView
?
@lilleyse This should be, yes. I'll close the issue
The following is a simple test glTF asset that uses the
KHR_texture_transform
extension:textureTransformTest.zip
Trying to load this with
cesium-native
will create an invalid model: It will not create a validAccessorView
for the texture coordinates.The first part of the issue is that the code block in
applyKHRTextureTransform::processTextureInfo
does not work when the data is interleaved. My understanding of this code block, based onthequickly skimming over it is...// Inlined comments
Now, these 'copies' that are made there fall apart when the data is interleaved: They receive wrong byte offsets and byte strides. Inserting
at the end of that block ""fixes"" it insofar that the proper data can be accessed, but of course, that's not a viable solution: The old buffer will still contain the (now unused) accessor data (interleaved into the data that is still used).
Even if this is fixed, I think that the implementation of
applyKHRTextureTransform::applyKHRTextureTransform
itself is broken: It "bakes" the texture transform that is defined in the texture into the texture coordinates of the mesh primitive. This will probably have undesired effects in several cases:KHR_texture_transform
extension, and the other one has a texture that does not have this extension. Modifying the texture coordinate accessor values will affect both primitivesKHR_materials_variants
). TheapplyKHRTextureTransform
method will only handle the default material, and bake its transform into the texture coordinates accessor, affecting the texture coordinates for the other materials as wellbaseColorTexture
with a certainKHR_texture_transform
, and ametallicRoughnessTexture
with anotherKHR_texture_transform
. TheapplyKHRTextureTransform
method will apply the transforms to the texture coordinates, one after another. (Even if the texture transforms are equal for both textures, the result of applying them twice will be wrong for both of them)One possible workaround might be to be to load these models with
gltfReaderOptions.applyTextureTransform = false
(which seems to return a "valid" model, from a first, quick test). After that, the texture transform would have to be applied manually, for each combination of ameshPrimitive.attributes["TEXCOORDS_n"]
accessor and a certaintexture
. (I'll try to draft a few lines of code for this later...).The following is an excerpt from the code that I just hacked into
cesium-cpp
for a quick test:In the current state, it prints "No valid texCoordView in primitive" because the texture coordinates accessor is not valid (
BufferTooSmall
).When applying the
// XXX
fix shown above and loading the following asset....textureTransformTestTwoTextures.zip
which contains one material with two textures (that both have the texture transform extension), a it prints
indicating that the texture transform was applied twice to the same texture coordinate accessor.
(With
options.applyTextureTransform = false
it seems to work, but the transform will have to be applied manually at the right place...)