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.

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

check texture names against safe known extensions #3

Closed
wants to merge 1 commit into from

Conversation

@illwieckz
Copy link
Member

commented Apr 4, 2017

fix #1

Show resolved Hide resolved sloth.py Outdated

@illwieckz illwieckz force-pushed the illwieckz:knownexts branch from 10e2ae1 to a7d27ac May 26, 2017

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented May 26, 2017

for documentation purpose, that was how looked the error before this fix:

Traceback (most recent call last):
  File "…/sloth.py", line 1103, in <module>
    sg.generateSet(path, setname = a.root, cutextension = a.strip)
  File "…/sloth.py", line 643, in generateSet
    self.__analyzeMaps(shader)
  File "…/sloth.py", line 399, in __analyzeMaps
    img = Image.open(shader["abspath"]+os.path.sep+shader["diffuse"]+shader["ext"]["diffuse"], "r")
  File "/usr/lib/python3/dist-packages/PIL/Image.py", line 2349, in open
    % (filename if filename else fp))
OSError: cannot identify image file '…/UnvanquishedAssets/src/tex-exm_src.dpkdir/textures/shared_exm_src/floor_grate2_trans_d.xcf'
Show resolved Hide resolved sloth.py
Show resolved Hide resolved sloth.py
@Viech

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Sorry for the delay.

  1. Sloth only reads diffuse and addition maps within __analyzeMaps. Normal maps and specular maps are not read at any point as far as I can see. Hence, you might want to limit your checks to those file types that are actually read, for performance reasons. I think it would be best to do that within __analyzeMaps where they are read anyway, so as to not duplicate the effort.

  2. Even if Sloth cannot open a diffuse or normal map, it should not be entirely ignored. The result in this case is merely a loss of quality for the generated shader. (See __analyzeMaps for why texture maps are read.) Running sloth on a folder of crn-compressed textures, for instance, should be possible.

  3. On the other hand, when a diffuse or normal map cannot be read, it would be good to output something to make the user aware of that quality loss. Maybe not for every file individually unless verbosity is requested, though.

Note that with my proposed changes, this PR does not fix #1 anymore. The real problem behind #1 is that the input can be ambiguous. Not just if one file is a .xcf source file and the other is the file that shall be used, but also if multiple non-source files with different extensiosn are present for some reason. There are a number of solutions to this I can think of:

  1. Abort whenever you find two files that only differ in their extension, alarming the user. After all, this is a case of bad input.

  2. Have a priority list of known extensions but abort if none of the existing extensions appears in it. Allows for keeping source files around, like in #1.

  3. Try opening files with duplicate basenames in any order until you can open any one of them. Do not abort as its fine if Sloth cannot read any one of them, as outlined above. Notify the user of the choice.

I think I'd favor (1) or (2) for simplicity.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2017

  • Abort whenever you find two files that only differ in their extension, alarming the user. After all, this is a case of bad input.

Hmm, having both the .xcf source and the .png texture in repository is not a bad input, it's the expected behavior.

  • Have a priority list of known extensions but abort if none of the existing extensions appears in it. Allows for keeping source files around, like in #1.

This solution relies on a well-known format/extension list. Well, the only need I have is to exclude source files (.xcf, .psd, .ora…), and it looks easier to blacklist the small list of them than whitelisting the non-source files. Once .xcf and others are blacklisted, I agree, having two entries for the same file (rock_d.png, rock_d.tga) can be considered as a bad input.

  • Try opening files with duplicate basenames in any order until you can open any one of them. Do not abort as its fine if Sloth cannot read any one of them, as outlined above. Notify the user of the choice.

The current solution (trying to read file and intercept exception if it fails) is going on that 3rd path, but do we need something very complex ? My first priority is to get legit use case working (like a source repository with .xcf files alongside diffuse ones), making Sloth a complete debugger is another job (would be cool by the way).

@Viech

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

The main issue I have with your current approach is that you are completely ignoring files that Sloth can't read, even though Dæmon may be able to. For instance, if one only has .crn files available, Sloth should do the best job possible, that is generate Shaders without the few extra features granted through file reading, so far being only color-variants and transparent surfaces.

The second issue I have with your approach is that you load every single texture map, which is slow/expensive.

Apart from that, I'm not asking for a debugger, the solution can be simple! 😉

If you want to maintain a blacklist, that's fine by me. In case you dislike hardcoding either a white- or a blacklist, feel free to add a command line argument to specify the white- or blacklist, with some sensible default argument.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

I'm closing this PR because obsolete and the thread is going the "too much" way. See #6 for an alternative fulfilling the initial need.

Other improvements talked about may be done as dedicated PR instead.

@illwieckz illwieckz closed this Jul 30, 2019

illwieckz added a commit to illwieckz/Sloth that referenced this pull request Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.