- 
                Notifications
    You must be signed in to change notification settings 
- Fork 65
tr_image: rework some builtin images #1804
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
Conversation
… forget about unused aliases
| As said here: Previously  Because all its channels were set to  But when stored as RED the alpha channel is opaque (so,  But I have seen no map in the whole betaserv corpus that used any  Actually it bugged me that the black image was not opaque black. So, we better have a dedicated  | 
| Also this may spark some discussions. First, the reason why there was no  It also means ioq3 never parsed  It means we really don't have to parse all those variants. I grepped etlegacy and realrtcw and none of them did more than defining  This also brings the topic that we may just name images like  If I'm right, we don't need to actually parse things like  So an alternative code may be to only parse for  | 
| The only  In fact, ioq3 and QIII don't parse  | 
| Hmm, actually if we name the images like  | 
| For a transparent shader we should make it possible to parse a shader with 0 stages (instead of assuming it is an error or a fog). Using a stage with an image is wasteful since it still has to execute the shaders. | 
| 
 Are you sure? For me it works with  | 
| 
 Test that latest commit. It will even break the console because some shaders rely on  | 
| Hmm, the problem is not the   | 
| Oh right, the code to treat builtin images specially (by allowing params mismatch) assumes that the builtin image names start with  | 
| 
 If it's currently assumed to be fog, then changing that might break some maps. | 
| A fog has to have fogparms so it's not hard to distinguish. | 
| 
 True. As long as it takes precedence over the automatic transparency. | 
d71246d    to
    c6a9493      
    Compare
  
    | I renamed all the builtin images to  I also numberized cinematic images, and slightly renamed some others. I removed aliases that were not used. | 
c6a9493    to
    a29693c      
    Compare
  
    | I also removed the patches modifying the builtin  Now this PR is about unifying and cleaning-up the builtin image code. | 
| Could we have a naming convention that distinguishes between images which are intended to be available for artists to use in shaders, and ones that are an internal implementation detail? Like the white image is good to use in shaders, but the light tile images aren't. I'm not saying that we should make it impossible to use those in shaders (it's convenient for debugging sometimes), but some convention to know whether it's supported or not would be nice. | 
| I also thought about that. I don't really like the  I believe it's a good idea to keep the  In names usable in shaders, we may even be more precise, using one syntax for generic name for context-dependent pseudo-variables (like  What would you suggest for truly internal things like a framebuffer? I dislike  What's your own opinion for RmlUi generated images? It seems to fall in the the case of not being intended for use outside of debugging, so it would use the same convention as framebuffers. | 
a29693c    to
    3c67504      
    Compare
  
    | I renamed possibly reusable images as  I left  | 
| I only kept the backward compatibility for  We actually use or have used  I have seen some games like Warsow (QFusion engine) using  | 
3c67504    to
    044d51c      
    Compare
  
    | I added a commit making  I also added a commit NUKING  | 
044d51c    to
    3643f8b      
    Compare
  
            
          
                src/engine/renderer/tr_image.cpp
              
                Outdated
          
        
      | // red | ||
| Vector4Set( data, 255, 0, 0, 255 ); | ||
|  | ||
| tr.redImage = R_CreateImage( "$red", ( const byte ** ) &dataPtr, 1, 1, 1, imageParams ); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also delete these fields from the tr struct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
        
          
                src/engine/renderer/tr_image.cpp
              
                Outdated
          
        
      | switch ( image->name[ 0 ] ) | ||
| { | ||
| return image; | ||
| case '_': // TODO: Remove once _fog is removed. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove _ now since it's not an image anyone needs to load from a shader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also this _fog image is now NUKED anyway.
…sn't used anymore Also drop the check for / as files cannot have a * or $ in their names.
3643f8b    to
    3c4b1fa      
    Compare
  
            
          
                src/engine/renderer/tr_image.cpp
              
                Outdated
          
        
      | return image; | ||
| case '*': | ||
| case '$': | ||
| if ( !strchr( image->name, '/' ) ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop the / test since * or $ can't be used in file names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned-up.
3c4b1fa    to
    0fbfbd1      
    Compare
  
    | LGTM | 
*name1×1$name, forget about unused aliases*name$name*name$blackbuiltin image opaqueOriginal message:
_blackopaque_nullfor zeroed image (instead of_black)_cinematiccolor instead of relying on previously set valuesimported from Add support for the GL_RED format, use it, also rework format detection #1793
1×1sized, makes the code simpler$blueimage.This will conflict with #1793 but I plan to rebase #1793 over it once this is merged.