SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
39 stars 10 forks source link

Issues with truncated returned file names with SUTextureGetFileName() #429

Closed jgmaxon closed 3 years ago

jgmaxon commented 4 years ago

API Issues Only

Bug Reports

Please include the following:

  1. SketchUp/LayOut Version: 2020
  2. OS Platform: Windows and macOS

Describe briefly what you are doing and what is happening. The SUTextureGetFileName() call sometimes does not return the correct file name, but instead a truncated name. Often, the extension is fully missing or only the . remains (usually '.png' or 'png' can be added and fixes the issue). Sometimes even some of the characters from before the .png extension are also missing. On macOS, sometimes some or all of the original path to the texture bitmap file is present, so I have to search for slash or backslash from the right to the left and take only the characters after it to get the file name.

Describe what the expected result is. The full file name, without the path, like is usually the case with most test files. Grab enough skp files from 3D Warehouse that use bitmap textures in its materials, and you will find examples of the issue I'm describing. The texture file names usually have spaces and other unexpected symbols, like asterisk (*), pound (#), etc.

// Complete reprodusible code snippet. (Important!)
        SUTextureRef texture = SU_INVALID;

        SUResult materialGetTextureError = SUMaterialGetTexture(material, &texture);

        if (materialGetTextureError == SU_ERROR_NONE)
        {
            // Texture file name

            // ***** TEST *****
            SUStringRef test;
            SUSetInvalid(test);
            SUStringCreate(&test);

            if (SUTextureGetFileName(texture, &test) != SU_ERROR_NONE)
            {
                return;
            }

            // UTF16

            Int lengthUTF16 = 0;
            if (SUStringGetUTF16Length(test, (size_t*)&lengthUTF16) != SU_ERROR_NONE)
            {
                return;
            }

            maxon::Utf16Char* tempUTF16CharString = nullptr;

            // Add one to the length to include a null at the end.
            tempUTF16CharString = NewMemClear(maxon::Utf16Char, lengthUTF16 + 1) iferr_return;

            Int returnedLengthUTF16 = 0;
            if (SUStringGetUTF16(test, lengthUTF16, (unichar*)tempUTF16CharString, (size_t*)&returnedLengthUTF16) != SU_ERROR_NONE)
            {
                return;
            }

            String stringUTF16(tempUTF16CharString, -1);

            DeleteMem(tempUTF16CharString);

            // UTF8

            Int lengthUTF8 = 0;
            if (SUStringGetUTF8Length(test, (size_t*)&lengthUTF8) != SU_ERROR_NONE)
            {
                return;
            }

            maxon::Char* tempUTF8CharString = nullptr;

            // Add one to the length to include a null at the end.
            tempUTF8CharString = NewMemClear(maxon::Char, lengthUTF8 + 1) iferr_return;

            Int returnedLengthUTF8 = 0;
            if (SUStringGetUTF8(test, lengthUTF8, (char*)tempUTF8CharString, (size_t*)&returnedLengthUTF8) != SU_ERROR_NONE)
            {
                return;
            }

            String stringUTF8(tempUTF8CharString);

            DeleteMem(tempUTF8CharString);
            SUStringRelease(&test);

            // Save out texture
            SUTextureWriteToFile(texture, "C:\\Temp\\test.png");

            // ***** TEST *****

            CSUString sketchUpTextureFilenameString;
            if (SUTextureGetFileName(texture, sketchUpTextureFilenameString) != SU_ERROR_NONE)
            {
                return;
            }

            // Required to extract the directory and file name separately
            Filename importedTextureFilename = sketchUpTextureFilenameString.Utf16();

            // TODO: (Jgaspe) Used to debug texture path issues
            // GePrint("" + importedTextureFilename.GetString());

            BaseChannel* colorChannel = importedMaterial->GetChannel(CHANNEL_COLOR);

            if (colorChannel)
            {
                BaseContainer colorContainer = colorChannel->GetData();

                // Set the texture path

                // Check that the file exists with the given name, and if not, try to find it and fix the texture path file name accordingly.
                if (!GeFExist(_textureDirectory + importedTextureFilename))
                {
                    // TODO: (Jgaspe) Used to debug texture path issues
                    GePrint("Broken texture path before:" + importedTextureFilename.GetString());

                    Filename pathToTextureFile = _textureDirectory + importedTextureFilename;

                    // The call requires a UTF-8 file path string pointed to by a character array pointer.
                    maxon::AutoMem<Char> skpPathToTextureFileCharStr = pathToTextureFile.GetString().GetCStringCopy(STRINGENCODING::UTF8);

                    if (skpPathToTextureFileCharStr == nullptr)
                        return maxon::OutOfMemoryError(MAXON_SOURCE_LOCATION);

                    if (SUTextureWriteToFile(texture, skpPathToTextureFileCharStr) != SU_ERROR_NONE)
                    {
                        return;
                    }

                    if (SUTextureSetFileName(texture, skpPathToTextureFileCharStr) != SU_ERROR_NONE)
                    {
                        return;
                    }

                    //importedTextureFilename = FixTexturePathFilename(importedTextureFilename);

                    GePrint("Broken texture path after:" + importedTextureFilename.GetString());
                }

<irrelevant code beyond this point>

For "Hearst+Tower+(New+York).skp":
"Hearst Tower by Sebastian S" instead of "Hearst Tower by Sebastian S.png"
"Hearst Tower by Sebastian S. Fence" instead of "Hearst Tower by Sebastian S. Fence.png"
"Hearst Tower by Sebastian S. Entrance" instead of "Hearst Tower by Sebastian S. Entrance.png"
"Hearst Tower by Sebastian S. 04" instead of "Hearst Tower by Sebastian S. 04.png"
"Hearst Tower by Sebastian S. 05" instead of "Hearst Tower by Sebastian S. 05.png"
"Hearst Tower by Sebastian S. 01" instead of "Hearst Tower by Sebastian S. 01.png"

[Hearst+Tower+(New+York).zip](https://github.com/SketchUp/api-issue-tracker/files/4176328/Hearst%2BTower%2B.New%2BYork.zip)

Attach any relevant files.

thomthom commented 4 years ago

Can you share the "Hearst+Tower+(New+York).skp" test model please?

jgmaxon commented 4 years ago

Funny, I had uploaded the file, but it's as though it's not there. I'll try again.

jgmaxon commented 4 years ago

Hearst+Tower+(New+York).zip

jgmaxon commented 4 years ago

I now see it in its own message above, Hearst+Tower+(New+York).zip. It's originally from the 3D Warehouse.

jgmaxon commented 4 years ago

https://3dwarehouse.sketchup.com/search/?q=hearst%20tower%20by%20sebastian%20s

thomthom commented 4 years ago

hm... not sure if this is a bug...

There's a number of reasons the filename might not be complete:

Looking at Ruby it appear to also return incomplete filenames:

GEIF257.jpg
Hearst Tower by Sebastian S
Hearst Tower by Sebastian S
Hearst Tower by Sebastian S
Hearst Tower by Sebastian S
Hearst Tower by Sebastian S
Hearst Tower by Sebastian S#1.jpg
Hearst Tower by Sebastian S#8.jpg
Hearst Tower by Sebastian S#8.jpg
Hearst Tower by Sebastian S#8.jpg
Hearst Tower by Sebastian S. 01
Hearst Tower by Sebastian S. 01
Hearst Tower by Sebastian S. 01
Hearst Tower by Sebastian S. 04
Hearst Tower by Sebastian S. 05
Hearst Tower by Sebastian S. Entrance
Hearst Tower by Sebastian S. Entrance
Hearst Tower by Sebastian S. Entrance
Hearst Tower by Sebastian S. Entrance
Hearst Tower by Sebastian S. Entrance
Hearst Tower by Sebastian S. Entrance
Hearst Tower by Sebastian S. Fence
Hearst Tower by Sebastian S. Fence 01.png
Hearst Tower by Sebastian S. Metal.jpg
Hearst Tower by Sebastian S. Roof 01.jpg
Hearst Tower by Sebastian S. Roof.jpg
[Carpet_Diamond_Olive]#1.jpg
[Carpet_Diamond_Olive].jpg
[Carpet_Diamond_Olive].jpg
h10.jpg
temp.jpg
temp.jpg
temp.jpg
temp.jpg
temp.jpg
temp.jpg
temp.jpg
temp.jpg
temp.jpg

But from what I can tell, this is all the info the model has.

Do you have an example where a current version of SketchUp isn't preserving the expected filename?

thomthom commented 4 years ago

Ah... the code will append the file extension based on image data if it's missing. However, since the filename has a period in it "Sebasitan S." it tricks the logic to thinking there is a file extension " Entrance".

thomthom commented 4 years ago
"Hearst Tower by Sebastian S" instead of "Hearst Tower by Sebastian S.png"
"Hearst Tower by Sebastian S. Fence" instead of "Hearst Tower by Sebastian S. Fence.png"
"Hearst Tower by Sebastian S. Entrance" instead of "Hearst Tower by Sebastian S. Entrance.png"
"Hearst Tower by Sebastian S. 04" instead of "Hearst Tower by Sebastian S. 04.png"
"Hearst Tower by Sebastian S. 05" instead of "Hearst Tower by Sebastian S. 05.png"
"Hearst Tower by Sebastian S. 01" instead of "Hearst Tower by Sebastian S. 01.png"

How do you assert what the filenames should be?

jgmaxon commented 4 years ago

Hi Thomthom,

What happens is that the TextureWriter helper calls output the PNGs, JPEGs, etc. with different names that look correct. However, SUTextureGetFileName() won't return the matching name. That means the full file name does exist somewhere in the .skp file data because the TextureWriter uses the correct one, but SUTextureGetFileName() just doesn't.

What then happens is that I check to see if the name returned matches any of the files at the path that I outputted the TextureWriter files to, and if not, I try to adjust it using some rules of thumb (add .png, add numbers at the end of the file name, compare the dimensions in case it's a mismatch, etc.) to see if I can reconstruct the correct file name. Since some vary only by the last few characters, truncated mismatching names will appear, making match up impossible by rules of thumb. What you get are the wrong textures applied, so the model looks obviously wrong. We then expect users to adjust the texture names and re-export / import an updated skp file, or fix the mismatched paths in Cinema 4D's material data.

Hypothetical Example: The files are exported by TextureWriter as: Foobar1.png Foobar2.png

SUTextureGetFileName() returns just "Foobar" for both materials that one should just be Front and the other Back. Try as I might, the best my 'rule of thumb' code can do is match the 'Foobar1.png' to both, unless they have different dimensions and my code gets to skip the first for the second and match it. If they have the same dimensions, I end up with 'Foobar1.png' matching for both materials.

Although we didn't get that many complaints over the years, as I think our users know to use reasonable texture file names and probably exchange the original texture files externally, internally at Maxon we (myself and the QAs) ran into this issue frequently with models from 3D Warehouse. However, we still like to minimize defects and quirks in our software and do not want beginners to get a bad early impression of our software if they import SketchUp files from public sources like 3D Warehouse and end up with messed up results.

It would also be best if this issue is fixed at its origin, rather than having to recode our SketchUp importer to export all the texture data into bitmap files from scratch.

Thanks for looking into this, BTW. Any info and help is appreciated.

Joey

thomthom commented 4 years ago

What happens is that the TextureWriter helper calls output the PNGs, JPEGs, etc. with different names that look correct.

Oh, that's interesting. maybe it is an bug after all in both the C and Ruby API. I'd have to dig deeper using the debugger to figure out what's going on.

thomthom commented 4 years ago

Looks like texture writes doesn't have access to any other source of filename, but is better at appending the file extension based on the image data type.

sketchupbot commented 4 years ago

SU-45405

sketchupbot commented 4 years ago

SKOR-12780

DanRathbun commented 3 years ago

Fix-2021.0

thomthom commented 3 years ago

Thanks for catching that Dan. The bot for some reason didn't apply the label... 🤔