x11 major code cleanup

Collapse
X
 
  • Time
  • Show
Clear All
new posts
  • calris
    Adept
    • Mar 2016
    • 194

    x11 major code cleanup

    Hi Guys,

    I've spent the last few days cleaning up the x11 front-end. You can find the changes here (it's not on a branch - I'll be paying the price for that later I'm sure)

    In a nutshell:
    • MAJOR update of the coding style - the code is barely recognisable, but how it actually works has barely changed
    • Split the core X11 handling (anything to do with Display or Window) into x11-util.[h,c] - most of the functions here are x11_[display, window, color, font]_xxx(). There are a few ragedy names that still need a little finessing
    • All those ugly global have been removed
    • The Angband Term interface functions are now all x11_term_xxx()
    • Saving of window positions has been restored
    • The ALT modifier now works!!!
    • Lots of redundant/spurious/legacy code has been stripped out completely


    The double-linked mapping between the Angband term structure and the x11 term data structure has been removed (this is a fairly significant change) - Now, an x11_term_data structure is allocated and assigned to term->data during term_data_init() - I believe this to be a much cleaner (and more in the spirit of having the data pointer in the term structure). The Angband term structures themselves are allocated in init_x11().

    The x11 frontend is poised for a complete removal of the ANGBAND_TERM_MAX window limit, but that is going to impact violently on the Mac and SDL front ends (but not Windows???)

    Anyway, take a look at what I've done - I'm more than happy to take the reigns of the x11 front end and fixing any bugs in it
  • Nick
    Vanilla maintainer
    • Apr 2007
    • 9647

    #2
    The general thrust of this is looking good. I haven't looked through it very closely, I'm afraid - I'm knee-deep in the curses feature branch, and RL is also a bit intrusive.

    I will await others' comments, but all your changes seem to me to be heading in the right direction. Beyond the front ends, he main implications for removing ANGBAND_TERM_MAX are on the subwindow options screen, I think - the general idea of removing hardcoded limits I am strongly in favour of. And I certainly like making the x11 structures more like the other front ends - it has always been a pain having to re-learn the x11 stuff to make one small change.

    If you are taking charge of this front end, here are a couple of things to think about:
    1. Graphics had to be removed because the code was not GPL compliant - takkaria knows the details of this better than me. Having them back in would be nice.
    2. Back around version 3.0.3, when the borg was still operational, a colleague of mine did some hacks to make the borg tie into xscreensaver. I have the basics of this here, based on roughly 3.3.0 code - possibly useless because of changes to input handling. It would be kind of nice to have xscreensaver support code in the x11 front end (a bit like the screensaver code in main-win.c) for if/when we have a functioning borg again, but this is really very speculative.


    Don't take particularly number 2 too seriously
    One for the Dark Lord on his dark throne
    In the Land of Mordor where the Shadows lie.

    Comment

    • AnonymousHero
      Veteran
      • Jun 2007
      • 1393

      #3
      Originally posted by Nick
      (Bits about sxscreensaver)

      Don't take particularly number 2 too seriously
      I would be amazed if anyone is actually using a screensaver these days -- other than "blank", obviously. I would even go so far as to argue that it would in fact morally wrong to do so given the amount of electrical power you would be wasting[1]. The only screensavers that this argument would (perhaps) not apply to would be Folding@Home and such.

      [1] Obviously, this would mean that Angband would be morally wrong to include support for it.

      Likewise, please take this as tongue-in-cheek. Though I actually do mean the underlying point seriously, I'm perhaps being a bit hyperbolic .

      Comment

      • calris
        Adept
        • Mar 2016
        • 194

        #4
        OK, I did a naughty - I moved the x11 patches to a new branch (x11-rework) and did a forced push into my github repo. So if you pulled my repo, sorry, but I busted your copy.

        From now on, you can get any x11 changes from here

        Comment

        • calris
          Adept
          • Mar 2016
          • 194

          #5
          Originally posted by Nick
          • Graphics had to be removed because the code was not GPL compliant - takkaria knows the details of this better than me. Having them back in would be nice.
          Just having a look at this now - If I get this working, somebody is going to owe me a beer - A very, very good beer - definitely NOT a VB or Fosters!

          Comment

          • Nick
            Vanilla maintainer
            • Apr 2007
            • 9647

            #6
            Originally posted by calris
            OK, I did a naughty - I moved the x11 patches to a new branch (x11-rework) and did a forced push into my github repo. So if you pulled my repo, sorry, but I busted your copy.

            From now on, you can get any x11 changes from here
            Excellent. I'll wait and see how you go with the graphics
            One for the Dark Lord on his dark throne
            In the Land of Mordor where the Shadows lie.

            Comment

            • calris
              Adept
              • Mar 2016
              • 194

              #7
              Originally posted by Nick
              Excellent. I'll wait and see how you go with the graphics
              Don't hold your breath - I've got no idea what I'm doing

              Well not entirely true - I've got code in the x11 front end to process the tile preference files and activating the term->pict_hook() function. But from here I'm a bit lost... Two things I need to sort out
              • Figuring out how the x and y coordinates x11_term_pict() relate to the term window. I've got the main window still drawing the text for the basic player stats on the left, but x11_term_pict() [i]appears[i] to use a new x=0,y=0 origin (which kind of makes sense, since the tiles will not be the same size as the fonts). My problem is, when a window is resized, we call Term_resize(cols, rows) which simply does not make sense when we have rows and columns with elements of two different sizes (fonts and tiles)
              • Loading the PNG, converting into a bitmap, and drawing a tile from the PNG onto the screen - This I think I can work out. The Windows front end uses the cross-platform libpng, so I can recycle code there and I'm sure there are plenty of x11 examples of copying images from one window to another. Colour palette may be an issue


              EDIT: this seems to imply we just give the term dimensions in tile sizes and then place text according to the tile grid - I can do better than that
              Last edited by calris; May 6, 2016, 06:54.

              Comment

              • calris
                Adept
                • Mar 2016
                • 194

                #8
                Progress

                EDIT: So the forum compressed the bejesus out of my screenshot... Basically I have the x11 front end processing the tile pref files, loading the select tileset, converting it to an XImage, and then plastering on the main window every time the term code asks to draw a tile. Now all I need to do is extract the x/y position of the request tile and past it on the right spot
                Attached Files

                Comment

                • calris
                  Adept
                  • Mar 2016
                  • 194

                  #9
                  OK, I've pushed what I've done so far for x11 tile support into my x11-rework repo

                  There is a small change to main.c to get the -g arg working properly

                  I'm actually pretty impressed with myself - It's a patchwork of main-win.c and the pngbook files readpng2.c and rpng2-x.c

                  So far I've got the PNG file load and converted into an XImage and a rudimentary attempt at a t->pict_hook function. But something is seriously wrong with my understanding of how Angband term code maps to drawing tiles - the screenshot is of the town after I took a few steps - I was actually able to walk into a store

                  I think it is REALLY close

                  Hold the fort! - I just used another tileset (#1) and got the second screenshot! And tileset 3 gives me the third screenshot

                  Know I know it's close.

                  Can someone have a look at what I've done and see what needs to be done to make it 'perfect'

                  Cheers
                  Attached Files

                  Comment

                  • takkaria
                    Veteran
                    • Apr 2007
                    • 1951

                    #10
                    Hmm. I think the problem is that you're using the tile width and height instead of the text grid width and height to plot here. I think you should be using td->tile_width and td->tile_height instead to set w2/h2.
                    Last edited by takkaria; May 7, 2016, 02:38. Reason: fixed incorrect variable names
                    takkaria whispers something about options. -more-

                    Comment

                    • Nick
                      Vanilla maintainer
                      • Apr 2007
                      • 9647

                      #11
                      This is fantastic. I hope you realise what a dangerous precedent you've set for yourself:
                      1. Do unsolicited useful work
                      2. Get answered with a wishlist
                      3. Fulfil wishlist


                      From my point of view, you're serving as some kind of code oracle.
                      One for the Dark Lord on his dark throne
                      In the Land of Mordor where the Shadows lie.

                      Comment

                      • calris
                        Adept
                        • Mar 2016
                        • 194

                        #12
                        Fixed one major problem - I missed setting 'use_graphics' which reset_visuals() uses to processes the appropriate tileset preference files - hence why only tileset #1 worked.

                        So now all the tilesets are drawn using the correct tiles, but I have a few issues
                        • Resizing does not appear to effect to size of the tile display area - It always worked for text. I've adjust the x11 resize code to call Term_resize(x, y) based on the tile size rather than the font size
                        • The tile display window position is generally pretty messed up - I turned on 'Continuously Center Map' (I thought it would have terrible performance, but turns out to be not too bad) - The 'centre' of the map is somewhere way to the bottom right (if using tileset #3) - last three screenshots are essentially the same using the different tilesets.
                        • As I understand it, the room taken up on the left edge is basically the amount of room taken up if the text was the same size as the tiles - is this something the Windows 'use_graphics_nice' and/or 'td->bizarre' flags are used for to create a better text/graphics mix?
                        • You'll notice a couple of tiles to the right of DEX/CON - I was wondering what they were, and realised they are my equipment slots - would be nice to have these as text so they didn't waste so much space
                        • Somehow I've trash text only support


                        Patch has been pushed
                        Attached Files

                        Comment

                        • takkaria
                          Veteran
                          • Apr 2007
                          • 1951

                          #13
                          I think you're assuming Angband has more advanced graphics support than it actually does. Graphics in Angband are displayed on the same grid as the text and are stretched/squished to fit. So while you have nice square tiles displayed, the game doesn't know that.

                          We have some options - the globals tile_width and tile_height - which allow a single tile to be stretched over multiple text grid positions. e.g. if tile_width is set to 2, then after each tile horizontally a 'spacer' tile is drawn (I think with char&attr = 255, but I've never looked that closely) to give space for the frontend to stretch the tile. Similarly with tile_height = 2, it'll add a spacer grid underneath each tile (I think it's underneath, anyway - you can find out by setting those parameters in text mode and seeing the spacing). This is poorly documented and hacky, obviously. It's a system that's evolved over years to try and improve things without fundamentally redesigning stuff.

                          Anyway, I think this is the root of your problems. So your call to Term_resize() should be based on text size, not tile size, and you need some tile resizing code too. Either that or work out how to make graphics and text coexist better
                          Last edited by takkaria; May 7, 2016, 02:55.
                          takkaria whispers something about options. -more-

                          Comment

                          • calris
                            Adept
                            • Mar 2016
                            • 194

                            #14
                            Thanks takkaria - by setting the 'window tile' size the same as the 'tileset tile' size, everything renders nicely. Of course the text looks totally whacky, and using the 64x64 shockbolt tiles is out of the question. So two things would be nice:
                            1. Finding a way to scale the tiles in X - not fun, but I'll see what I can find
                            2. Split the Term window into 'dungeon view' and 'textual fluff' so the text can have a different size to the tiles

                            Comment

                            • molybdenum
                              Apprentice
                              • May 2013
                              • 84

                              #15
                              Originally posted by takkaria
                              Either that or work out how to make graphics and text coexist better
                              So, I actually did something for this a few years ago, but I never got it to a finished-enough state to push (it was also dependent on a lot of grubby OS X code I had rewritten which also wasn't ready). It's one of the things on my list to fix up and implement in the OS X overhaul, in addition to other grid things like big tiles.

                              The basic idea is that there are actually two grids: a logical grid (the term x/y/width/height) and a physical grid (the grid you are actually drawing to). When Angband wants to update a tile at logical (x, y), the conversion code checks if this coordinate is (a) in the main term and (b) in the region of the term that we should consider using separate tile sizes. In the case of OS X, the default was to use the text size as the grid size and the special region was the area not occupied by the message row, status row, and sidebar.

                              Once I got a good chunk of logic issues out of the way, it really wasn't that much code. I don't think I had gotten all of the edge cases though.

                              Comment

                              Working...
                              😀
                              😂
                              🥰
                              😘
                              🤢
                              😎
                              😞
                              😡
                              👍
                              👎