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

Max Texture Name Length not working #6

Closed
bmsq opened this issue Jun 29, 2018 · 9 comments
Closed

Max Texture Name Length not working #6

bmsq opened this issue Jun 29, 2018 · 9 comments

Comments

@bmsq
Copy link
Contributor

bmsq commented Jun 29, 2018

I set the maxtexturenamelength property to a value greater than 8 and load textures from a directory containing PNG files.

I was expecting to see all textures in the texture browser and names reflecting the full file names.

Instead, where I have textures called "mmetal1_1.png" through to "mmetal1_8.png", I only see a single texture in the browser called "mmetal1_". When applying the texture to sidedefs or sectors, the truncated name is set. I can manually enter "mmetal1_5" in the relevant input box without triggering validation errors but the name is truncated to "mmetal1_" after applying my change to a sidedefs/sector.

@anotak
Copy link
Owner

anotak commented Jun 29, 2018

apologies, as of right now, that's essentially unsupported, despite it being an option in the configuration file added by a previous developer. i'm honestly surprised it doesn't crash. i would be very careful, i'd worry it might trash some of your data while saving. actually, i'm seeing now that there is a comment in the configuration file that implies such.

@bmsq
Copy link
Contributor Author

bmsq commented Jun 29, 2018

Thanks for the quick reply. I didn't notice the comment in the default configuration files, my mistake.

I use UDMF maps in my own non-doom engine, so I find this option very useful. Im not sure the reason why it's currently unsupported but would you be interested in a pull request if I re-added it?

@anotak
Copy link
Owner

anotak commented Jun 29, 2018

it's never actually been supported, unfortunately.

the reason for it is complicated. the short version is, basically 8 bytes = 64 bits, and so texture names are treated as 64 bit integer numbers all over the source code. the really bad part is i don't know a good solution to fix this while retaining compatibility with the plugin API (essentially, anything marked public can't have its function signature change). the other forks of doom builder 2, GZDB/GZDB-BF, got around this by using hashes. this approach has problems, as i examined some recently, if you see my post in this thread over here. i'm not 100% sure what to do here, like maybe i should just say "screw it" and break compatibility. or maybe i should just take the extremely small chance of a hash collision.

one possibility i thought of in the time since that post is only using a hash if the name is longer than 8 characters, and then setting the most significant bit. this increases the chance of a collision by some number that i don't know enough of the right math to be able to understand. but only for names longer than 8 characters or those which use characters larger than (char)0x7F. such characters aren't allowed in WAD files, so it wouldn't be a common case.

if you want to see the relevant bits of code, search for the confusingly named longname variable.

i need to get some sleep, i'll try to think about this more. idk what the right answer is right now.

@bmsq
Copy link
Contributor Author

bmsq commented Jun 30, 2018

I wonder what the original design was behind longname? It looks like it was an Id or handle but with Lump.MakeLongName() scattered everywhere, that would be one seriously leaky abstraction.

Swapping out the MakeLongName implementation with a hash function like GZDB isn't a bad quick fix but I agree regarding collisions. Murmur hash has even distribution properties, which is desirable but it means collisions are just as likely regardless of the maxtexturenamelength setting. That is a rather nasty side effect.

One option is to use the existing algorithm when maxtexturenamelength <= 8 and switch to something a little slower but robust when long names are enabled. Detecting and handling hash collisions would require keeping a table of all string names, its probably easier to maintain a dictionary<string, long> and just use a simple counter for allocating longnames for previously unseen names. Lump.MakeLongName appears to be mostly called during initialisation, so performance shouldn't be a big concern during editor use. The exception to this is the Sector and Sidedefs update routines. I assume they are only called when a user clicks "ok" on the relevant editor dialogs? If so, I'd be surprised if this approach would have a perceivable impact.

Any thoughts or suggestions before I start working on a pull request?

@anotak
Copy link
Owner

anotak commented Jun 30, 2018

OH, that's a good suggestion, but it leads me to a possible better version:

what if we use an index-based longname only if the current input lump is >8 name length, and then | the index with the sign bit (unchecked((long)0x8000000000000000)). this still gives us 2^63 indices, with no collisions. this does run into issues with > (char)0x7F first characters in names, but we can check for that case and use the index-based name there too.

regarding the performance aspects, yes, it is mostly only used during loading, but loading is actually an important place for performance for me (and users). MakeLongName()/MakeNames() can actually be a bottleneck in extreme-but-real-world cases. but this solution should be plenty fast enough. oh, on that note, also, be aware that MakeNames() also exists and creates a longname.

@anotak
Copy link
Owner

anotak commented Jun 30, 2018

oh before you get started: anywhere you comment (if you do), can you sign it with a prefix with a name or an abbreviation? exact formatting of the signature doesn't matter too much.

as far as any other code formatting stuff, idk, just use your best instincts. if i have a problem with it, i can clean it up after.

@bmsq
Copy link
Contributor Author

bmsq commented Jun 30, 2018

Using the most significant bit to separate "classic" and long texture names is an interesting idea. First character > 0x0F is in the extended ASCII range too which feels clean.

Since C# uses the MSB to indicate sign, indexes could actually be stored as negative numbers rather then using bit flags. Implementation and debugging would be simplified as the number could be directly interpreted without the need for bit logic.

@anotak
Copy link
Owner

anotak commented Jun 30, 2018

the only issue with that is that there is no -0, so either way you will be forced to do logic of some kind anyway. um, i guess i leave that to your preference?

@bmsq
Copy link
Contributor Author

bmsq commented Jul 5, 2018

Resolved by pull request #7

anotak added a commit that referenced this issue Jul 7, 2018
Added support for long texture names
@anotak anotak closed this as completed Jul 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants