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

wal texture embedded values aren't being displayed #3013

Closed
eGax opened this issue Feb 18, 2020 · 13 comments · Fixed by #3872
Closed

wal texture embedded values aren't being displayed #3013

eGax opened this issue Feb 18, 2020 · 13 comments · Fixed by #3872
Assignees
Labels
Prio:2 Medium priority: Non crash bugs that impede the user, features that add new functionality. Type:Bug Errors and problems
Milestone

Comments

@eGax
Copy link
Contributor

eGax commented Feb 18, 2020

I don't think TrenchBroom has ever processed or used the wal texture information embedded in wal textures as other map editors do. Each wal is capable of containing information about the texture's surface & content flags, light value, name and next texture in an animation sequence. The first 3 of those are usually read from the wal file and that information is passed on to the editor to check those flags and enter that light value without the need for user intervention.

Here is the Windows only texture creator/editor program named Wally with e1u1/brlava loaded from Quake 2 and Texture Information window open.

image

Here is netradiant-custom with a new Quake 2 map started, 1 brush made and the same texture from above applied. You can see the from the surface inspector the flags are checked and the light value entered without me manually doing it.

image

Now here I opened JACK, started a new Quake 2 map, created a brush and applied the same texture. You can see those flags automatically checked and light value of 700 entered again without me doing it.

image

Expected Behavior

I expect TB to handle the embedded wal file information as other popular map editors do.
Currently, the same texture applied to a brush in TB shows no embedded defaults:

image

Thank you very much!

@ericwa
Copy link
Collaborator

ericwa commented Feb 19, 2020

@eGax good find. Just so we're 100% sure what is going on, could you upload both the Netradiant-custom and JACK .map files?

I'm wondering if the editors are just displaying the information from the .mip files, or are they writing it to the map?

Another question, do both editors let you uncheck e.g. the Lava contents flag?

@eGax
Copy link
Contributor Author

eGax commented Feb 19, 2020

Yes, sorry I should have done this in the original post. Yes, both editors will allow the user to uncheck one or all the flags or change the light value. After conversing with @ericwa and niger for a bit on discord I have more to add here.

Based on the examples above here is the map brush exported from JACK in Quake2 220 format:

{
( -64 -64 16 ) ( -64 -64 -16 ) ( -64 64 16 ) e1u1/brlava [ 0 -1 0 0 ] [ 0 0 -1 0 ] 0 1 1 8 9 700
( 64 64 16 ) ( 64 64 -16 ) ( 64 -64 16 ) e1u1/brlava [ 0 1 0 0 ] [ 0 0 -1 0 ] 0 1 1 8 9 700
( 64 -64 16 ) ( 64 -64 -16 ) ( -64 -64 16 ) e1u1/brlava [ 1 0 0 0 ] [ 0 0 -1 0 ] 0 1 1 8 9 700
( -64 64 16 ) ( -64 64 -16 ) ( 64 64 16 ) e1u1/brlava [ -1 0 0 0 ] [ 0 0 -1 0 ] 0 1 1 8 9 700
( -64 64 -16 ) ( -64 -64 -16 ) ( 64 64 -16 ) e1u1/brlava [ -1 0 0 0 ] [ 0 -1 0 0 ] 0 1 1 8 9 700
( -64 -64 16 ) ( -64 64 16 ) ( 64 -64 16 ) e1u1/brlava [ 1 0 0 0 ] [ 0 -1 0 0 ] 0 1 1 8 9 700
}

Jack seems to write in the the flags and light values to the map file no matter what whether you have changed the default wal values from the surface properties window or not. Applying a new texture resets the values and inserts the ones from the embedded wal defaults you just picked.

Now here is the same brush with applied texture created in netradiant-custom:

{
( 64 64 16 ) ( 64 -64 16 ) ( -64 64 16 ) e1u2/brlava 0 0 0 1 1
( 64 64 64 ) ( -64 64 64 ) ( 64 64 -64 ) e1u2/brlava 0 0 0 1 1
( 64 64 64 ) ( 64 64 -64 ) ( 64 -64 64 ) e1u2/brlava 0 0 0 1 1
( -64 -64 -16 ) ( 64 -64 -16 ) ( -64 64 -16 ) e1u2/brlava 0 0 0 1 1
( -64 -64 -64 ) ( -64 -64 64 ) ( 64 -64 -64 ) e1u2/brlava 0 0 0 1 1
( -64 -64 -64 ) ( -64 64 -64 ) ( -64 -64 64 ) e1u2/brlava 0 0 0 1 1
}

By default n-c shows the flags and values, but doesn't write them to the map file unless you make changes to the embedded wal defaults from the surface inspector. If you leave the flags & value as default you can easily click on other textures in it's texture browser to change the brush texture and the flags & value will change the texture default embedded in the textures you are selecting as in this gif:

20

Once you change those defaults the values are written to the map file and the behavior shown above is no longer valid and you need to edit values by hand, recreate the brush and apply a texture, or remove the flags from the brush via a text editor. IMO this is bad behavior and I've already informed niger. Most likely he will add a small ui button for resetting values back to embedded defaults.

So there you have it. We need to look at the q2 compilers to see how they handle the flags/values, but from the looks of things, my hunch is they fall back on the embedded values in the wal textures only when the map file contains no info to override those defaults.

@eGax eGax changed the title wal texture embedded values aren't being used wal texture embedded values aren't being displayed Feb 19, 2020
@ericwa
Copy link
Collaborator

ericwa commented Feb 20, 2020

Here is the relevant bit of the Q2 qbsp:
https://github.com/id-Software/Quake-2-Tools/blob/master/bsp/qbsp3/map.c#L609

So, the behaviour is the following: if the plane definition line has the 3 tokens at the end (contents surf value), those are used and the texture's values are ignored; otherwise, the contents, surf, value from the texture are used as a fallback.

@ericwa
Copy link
Collaborator

ericwa commented Feb 20, 2020

One suggestion is to add a radio button:

  • use texture defaults
  • custom

above the flags/content/value. When you have "use texture defaults" selected, TB would display in the UI the values from the texture, with controls greyed out. When you select "custom" it makes the controls editable, copying the values from the current texture.

@kduske
Copy link
Collaborator

kduske commented Feb 20, 2020

My suggestions:

We need to keep track whether or not to use the defaults for each face, i.e. use an additional bool useDefaultsFromTexture or use an optional to hold the triple of surface flags / content flags / surface value.

When loading the map, useDefaults is set to true if the face didn't contain the three additional values, otherwise we set it to false. Same goes for saving the map.

In the UI, we do not expose useDefaults directly. If useDefaults is true, we show the defaults from the texture as if they were set by the user. When the user makes any change to any of the three values, we copy the defaults into the face attribs before applying this change. Now useDefaults is false, and we use the values as set by the user. When useDefaults is false, the UI behaves as it does now, essentially.

To reset useDefaults to true, we use the Reset button that we already have in the face inspector in order to not overload the UI with even more settings. So resetting sets useDefaults to true, and in the UI we show the default values again. When saving the face in this state, the three additional values are gone, and the compilers will use the defaults from the textures, too.

@ericwa ericwa added Prio:3 Low priority: Minor problems and nice to have features Type:Enhancement New features labels Feb 23, 2020
@ericwa
Copy link
Collaborator

ericwa commented Feb 23, 2020

The only concern I have is when that UI/data pattern shows up (I'm not sure if it has a name but I'll call it "Cascading settings") it should really be communicated visually: whether a value is user-set or coming from the defaults, because it affects the behaviour. This shows up in most "pro" software (IDE's, gamedev) and in TB with the attribute grid.

E.g. if useDefaults is true, changing the texture will also change the flags, but if useDefaults is false the flags will stay the same when changing textures. If there's no visual indication, it'll mean that the flags will appear to randomly change or stay the same when you change textures.

Maybe we could use the same visuals as the attribute grid to show useDefaults (grey italics text = coming from default, black non-italics = user set.) I'm not sure about the UI interaction for manipulating useDefaults.

@kduske
Copy link
Collaborator

kduske commented Feb 23, 2020

E.g. if useDefaults is true, changing the texture will also change the flags, but if useDefaults is false the flags will stay the same when changing textures. If there's no visual indication, it'll mean that the flags will appear to randomly change or stay the same when you change textures.

Yeah, this is a good point that I had not considered. It would not apparent to the user why the flags are changing (or not). I guess we need a separate checkbox then.

@ericwa
Copy link
Collaborator

ericwa commented Feb 23, 2020

BTW, Quark does exactly what you proposed, which strengthens the case that this overall approach is good.

This first screenshot is the useDefaults = true case, the 3 values are not written to the .map, and if I change textures the flags checkboxes update (but nothing is written to the map, still).
defaults

Touching the checkboxes (here I checked "light") locks that set of flags in place, so they no longer update when I change textures, and now they're written to the .map
user-set

@Paril
Copy link

Paril commented Aug 1, 2021

This is a rough one because Q2 mapping is nearly impossible with TB unless you manually set up all of the surface flags/brush contents/values per brush. Editing existing .map files can be destructive because it puts down 0/0/0 as the overridden values in the .map.

@eGax
Copy link
Contributor Author

eGax commented Aug 1, 2021

Good point. We had discussed this previously, but over writing the values of an existing map isn't good.

Was this just opening an existing map in TB, saving, and opening in a text editor to check?

@Paril
Copy link

Paril commented Aug 1, 2021

Yeah. The behavior of older editors (Qoole/QuArK) is what was described above: you don't write the three values out if they match the .wal file's values, essentially. If you, say, open up a random water brush and accidentally check MIST and then uncheck it, it will still not write the three values out because it still matches the ones defined in the .wal, but if they are different at all then they're thrown into the .map.

EDIT: the alternative is to just "lock" in the default values and unlock them if they are changed, but there's no sense in writing the same values that are in the .wal file out to the .map if they match anyways, and that will be easier on the UI since you don't need to distinguish visually between defaults and not. You just need a "reset" button and to fill in the defaults from the .wal, that's all.

@ericwa ericwa added Prio:2 Medium priority: Non crash bugs that impede the user, features that add new functionality. Type:Bug Errors and problems and removed Prio:3 Low priority: Minor problems and nice to have features Type:Enhancement New features labels Aug 2, 2021
@ericwa
Copy link
Collaborator

ericwa commented Aug 2, 2021

I'll see if I can take a look at this soon.. (yeah, agreed Paril, it's a silent map corruption bug in TB, so I've flagged it as a bug accordingly)

@ericwa ericwa self-assigned this Aug 2, 2021
@ericwa ericwa added this to the 2021.2 milestone Aug 2, 2021
@kduske
Copy link
Collaborator

kduske commented Aug 4, 2021

@ericwa I have a few thoughts on the design. I think we have other places where we want to store format-specific data along with the textures as well, so it might be nice to introduce a generic mechanism (variant member of Texture comes to mind).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prio:2 Medium priority: Non crash bugs that impede the user, features that add new functionality. Type:Bug Errors and problems
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants