full PLE mod for BUG

Yes, I found the same thing. And restarting Civ doesn't get rid of the ghost icons for you? It did for me. I'll see if I can fix this before we do the whole PLE rewrite thing.
 
I've started the coding for the UnitPlotList enhancements. I've committed the UnitPlotListUtil.py file (revision 1866).

Now, don't get carried away because nothing actually uses it yet.
 
I've started the coding for the UnitPlotList enhancements. I've committed the UnitPlotListUtil.py file (revision 1866).
Revision 1867 - updated UnitPlotListUtil.py with more stuff. While it only contains the promoframe at the moment, I think it is time to start folding this into the maininterface.
 
I mentioned/asked this over in the BUG Questions thread, a few days back. It would have some bearing on the PlotList primarily - as that seems to be what causes most folks slow-downs when using BUG.

I don't recall where I read it, one of the guides or some of the documentation that has been created in regards to CIV modding.

Would CIV still read/recognize the python files if they were compiled into .pyc files?
The only place I see them are in the directories that have files for the CvGameCoreDLL.dll. If it is possible then it stands to reason things like MainInterface screen and the like should get a speed boost.
 
Revision 1867 - updated UnitPlotListUtil.py with more stuff. While it only contains the promoframe at the moment, I think it is time to start folding this into the maininterface.

As per our other discussion, definitely do not fold this into BUG at this time--way too risky and also incomplete. Feel free to create a branch in SVN for this work, however, and go to town. :)

Would CIV still read/recognize the python files if they were compiled into .pyc files?

Sorry, I forgot to reply to this the other day. Precompiling Python files would only help speed up loading of Python. All Python files are compiled eventually, and this is not the cause of the slowness issues. I have no idea if Civ4 would recognize .pyc files, but I do believe that they would make no difference if so.

The slowness with PLE isn't even the Python. The Python code is trivial. The slowness is that it draws/hides so many overlapping graphical elements, and I would be that the Civ4 engine redraws the screen with each call that changes the graphics. This time is spent in the EXE layer which we cannot control. We'll attack the problem by limiting the amount of drawing we do by detecting when we don't need to redraw an element that hasn't changed.
 
Ah yeah, in my CivilizationIV.ini I posted though, there is a setting:
; Copy entire image each frame, not just dirty pixels
BinkCopyAll = 0

I believe that setting helps a lot more than most of the other tweaks in the .ini

Anyways, sorry for OT!! :)
 
As per our other discussion, definitely do not fold this into BUG at this time--way too risky and also incomplete. Feel free to create a branch in SVN for this work, however, and go to town. :)
I was just going to ruin my local copy. But I would value your 2c if you have time to take a quick look at what I have committed.
 
Sure, here are a few notes along the way.

Code:
localText = CyTranslator()

Use BugUtil.getPlainText() and BugUtil.getText() instead to pull translated strings out. The ones in BugUtil have options for having a default and translating our custom [ICON_FOO]s if needed. Of course, I doubt you'll be using them much.

Code:
iLeaderPromo = gc.getInfoTypeForString('PROMOTION_LEADER')
sFileNamePromo = ArtFileMgr.getInterfaceArtInfo("OVERLAY_PROMOTION_FRAME").getPath()

There are some cases where this gc-acquired stuff isn't available when the Python module loads. This is why I have init() functions in many BUG modules. You can either test to make sure these work or you can use an init() function for maximum safety. The latter ties the module to BUG, but that shouldn't be a problem here.

Code:
screen = CyGInterfaceScreen( "MainInterface", CvScreenEnums.MAIN_INTERFACE )

All of the Civ4 code I've seen does this each time the screen is needed, usually in a function called getScreen(). I have no idea if it's necessary. For CyGlobalContext, you can clearly create a single one in a global variable as you've done. We know that works. For CyEngine, I found I couldn't do that with the Strategy Layer for some odd reason; I had to create a new one each time I needed to update the screen. Be prepared for this to fail or start with a getScreen() function from the get-go.

Code:
# constants
sUpdateShow = "SHOW"
sUpdateShowIf = "SHOWIF"
sUpdateHide = "HIDE"
sUpdateNothing = "NOTHING"

I tend to favor int constants for things like these. They are called "enumerations" when you have a bunch of related values, for example the cardinal directions. For this I would use a little trick using range() so you don't have to assign a value to each manually:

Code:
(
    sUpdateShow,
    sUpdateShowIf,
    sUpdateHide,
    sUpdateNothing,
) = range(4)

This assigns 0, 1, 2, 3 to those variables. When you need to add a new value, just add a new line and change 4 to 5. It's nice because you can reorder the values if you want to merely by reordering the lines. Ints are generally preferable to strings because they are smaller (space) and cheaper to compare. Here that won't make a noticeable difference, but it's a good thing to practice I believe.

Code:
def updatePLEOptions():
	# Capture these for looping over the plot's units
	self.bShowWoundedIndicator = PleOpt.isShowWoundedIndicator()
	...

This function doesn't believe to a class, so you must remove the "self." prefixes and declare them as globals:

Code:
bShowWoundedIndicator = False
...

def updatePLEOptions():
    global bShowWoundedIndicator, ...
    bShowWoundedIndicator = PleOpt.isShowWoundedIndicator()
    ...

Keep in mind that you'll need to redraw the plot list (easy way) when any of these options change or hide/show just the changed option (hard way). If you want to start coding this module to be more extensible (add new elements later), you might want to generalize this.

Use the range() trick above to define an enumeration of Plot List Elements. Then map each option to an element. Finally, track which are visible in a list of booleans:

Code:
NUM_PLE_OPTIONS = 7
(
    WOUNDED_DOT,
    HEALTH_BAR,
    MOVE_BAR,
    GG_INDICATOR,
    PROMO_INDICATOR,
    UPGRADE_INDICATOR,
    MISSION_TAG,
) = range(NUM_PLE_OPTIONS)

PLE_OPTION_MAP = {
    WOUNDED_DOT: PleOpt.isShowWoundedIndicator,    # note no () after function
    HEALTH_BAR: PleOpt.isShowHealthBar,
    ...
}

pleOptions = [False] * NUM_PLE_OPTIONS

def updatePLEOptions():
    for element, option in PLE_OPTION_MAP.iteritems():
        pleOptions[element] = option()

Now when you add a new element, you only have to add a new constant in the range() block and a line to map it to the option accessor in the MAP. Note that I used PLE in the names, but clearly these apply to the plot list as a whole, so maybe drop PLE entirely from most things here except PLE-specifics.

Code:
## - the vanilla BtS unit plot list has a panel for each row of icons and the units are arranged from the bottom left
## - the PLE unit plot list has no panel (drawn straight onto the screen) and counts from the top left (when in standard format)
## - the BUG unit plot list has one big panel and counts from the bottom left
##    - this means that the max'th row fills up first, then the max'th-1, etc.

I would consider normalizing the use of y to be the same for all, with 0 at the bottom and increasing toward the top of the screen (or vice versa) to make the code much easier. You'll be writing all the code essentially from scratch by pulling pieces from CvMI and modifying it, so you may as well make that easier and reuse as much as possible.

As I explained before, I think we can get away with having a single draw function with a few plugin functions that calculate where the elements get drawn. For example, both Vanillla and PLE have a health bar. The only difference is where it gets drawn in relation to the unit's icon. You don't need two draw functions for it, merely call a getX() and getY() function within the single draw function. Each PlotList subclass would be able to override the getX/Y() functions and inherit the master drawing code.

Code:
class UnitList:
	def __init__(self, vScreen, vCols, vRows, yRes):
		screen = vScreen
		iCols = vCols
		iRows = vRows
		...

All these variables you are setting in __init__() are instance (class) variables and thus require "self." before them:

Code:
self.screen= vScreen
self.iCols = vCols
...

BTW, since these have "self." in front, you don't have to give them names different from the parameter names above. Change vCols to iCols, etc.

Code:
self.iCols = iCols

Here these are two separate variables with no danger of clashing. Once you get used to this it becomes easier, though it may seem error prone at the start. The only time you have the danger of making a typo is in the __init__ function itself.

Code:
iCols = iCols

Clearly this does nothing, but you'd catch it as soon as some other function tried to access "self.iCols" with "AttributeError: object has no attribute iCols".

Code:
		_x = 315 - 3
		_y = yRes - 169 - 3 + (1 - vRows) * 34
		_w = vCols * 34 + 3
		_h = vRows * 34 + 3

I get nervous when I see numbers in calculations like this. First, I see 34 in three places, which screams make me a constant!, probably ICON_SIZE or CELL_SIZE or perhaps it's not constant but a function getCellSize/Width/Height() that is based on option settings? The others like 315 and 169 look like they are based on resolution, and 3 is probably a spacing. Create constants for these so that the intent is clear.

Code:
class UnitPlot:
	def __init__(self, vBupPanel, iIndex, x, y):
		sBupString = sBupStringBase + str(iIndex)
		xPixel = _getxPixel(x)
		[B]yPixel = _getyPixel(x)[/B]

Oopsies! :D Also, "self."s are needed here too.

Code:
		# add promo frame
		screen.addDDSGFCAt(sBupString + "PromoFrame", vBupPanel, sFileNamePromo, xPixel + 2, yPixel + 2, 32, 32, WidgetTypes.WIDGET_PLOT_LIST, iIndex, -1, False )
		screen.hide(sBupString + "PromoFrame")

This should be extracted to a create() function. I think here is where you have a great opportunity for abstraction and generalization that will save you a lot of coding. Create a separate object for each element. I'll write more about this tomorrow as it will get pretty complex.

Code:
	def _getxPixel(self, x):
		return x * 34 + 3

Again, use constants. These are probably the same 34 and 3 as above. By using a constant/function, if we ever change it we can do so in one place.

Code:
_updatePromo(sBupString + "PromoFrame", pCurrUnit, pPrevUnit, sUpdateHide)
_updatePromo(sBupString + "PromoFrame", pCurrUnit, pPrevUnit, sUpdateNothing)
_updatePromo(sBupString + "PromoFrame", pCurrUnit, pPrevUnit, sUpdateShowIf)
_updatePromo(sBupString + "PromoFrame", pCurrUnit, pPrevUnit, sUpdateShow)

I'll get more into this tomorrow, but here I would more likely use different functions rather than a parameter for what to do. Also, I would have the function create the element ID (what if an element has two graphics?) and access pCurrUnit and pPrevUnit via the self object, so you don't need to pass them in:

Code:
if not pCurrUnit:
    if not pPrevUnit:
        # both have no unit, nothing to do
    else:
        # empty -> full
        _drawPromo()
else:
    if not pPrevUnit:
        # full -> empty
        _hidePromo()
    else:
        # both full
        _updatePromo()   # this function doesn't do anything, but e.g. mission tag updates the image

Using separate functions makes the following code easier to write and read, and as you'll see tomorrow, this can have a huge payoff when you generalize the element functions into classes.

Code:
def _getPromoString():
	return self.sBupString + "PromoFrame"

def _drawPromo():
	if self.pCurrUnit.bPromo:
		self._showPromo()

def _updatePromo():
	if self.pCurrUnit.bPromo and not self.pPrevUnit.bPromo:
		self._showPromo()
	elif not self.pCurrUnit.bPromo and self.pCurrUnit.bPromo:
		self._hidePromo()

def _hidePromo():
	if self.pPrevUnit.bPromo:
		self.screen.hide(_getPromoString())

def _showPromo():
	self.screen.show(_getPromoString())

The UnitDisplay class looks great. The only slight impovement (not sure even if it is) I can make would be for the dot indicator. Instead of calculating and storing all the effects, store only the data that determine those effects later: GG (already done later), has moved, and injured (both booleans):

Code:
self.bMoved = pUnit...
self.bInjured = pUnit...

This way you push off doing the calculations and image-choosing to the last possible moment (when it must be drawn). When the data doesn't change (the part we're trying to optimize), you won't need to do those calculations. They would be done in the _drawDot() function instead of here and they'd use the bGG, bMoved, and bInjured values as you already, just in a different function.

As for _getMission(), let's finally hook in UnitUtil.getOrder() which returns an enumeration constant. We can then turn that constant into an image using a simple constant list.

This is a great start, and I'd say you're on a very good track.I'll go into more details tomorrow about what I mean when I say to abstract the elements into classes. Essentially you'll have a class for each element type (promo frame, upgrade indicator, mission tag, etc) that knows how to draw itself given a UnitDisplay (or two in the case of updating).

PlotList would have a constant list of these objects, and it would loop over them calling the appropriate function (hide, draw, or update) on each one.

Code:
if pPrevUnit:
    if pCurrUnit:
        # both full
        for element in elements:
            element.update(pPrevUnit, pCurrUnit)
    else:
        # full -> empty
        for element in elements:
            element.hide(pPrevUnit)
else:
    if pCurrUnit:
        # empty -> full
        for element in elements:
            element.draw(pPrevUnit)
    else:
        # both empty
        # do nothing

As you can see if we add a new element one of this code has to change. You only have to write the new element draw functions (same) and add it to the list of elements (1 line). Much easier. This is OOP at its best.
 
Firstly - thanks for all of the comments. I will review and update my file based on what you have suggested.

A couple of return comments ...

Code:
## - the vanilla BtS unit plot list has a panel for each row of icons and the units are arranged from the bottom left
## - the PLE unit plot list has no panel (drawn straight onto the screen) and counts from the top left (when in standard format)
## - the BUG unit plot list has one big panel and counts from the bottom left
##    - this means that the max'th row fills up first, then the max'th-1, etc.

I would consider normalizing the use of y to be the same for all, with 0 at the bottom and increasing toward the top of the screen (or vice versa) to make the code much easier. You'll be writing all the code essentially from scratch by pulling pieces from CvMI and modifying it, so you may as well make that easier and reuse as much as possible.

Part of me agrees, but the PLE action is to actually show the 'top' unit at the top left, while the vanilla BtS action is to show the 'top' unit at the bottom right. Also, the x,y for each index will change with the different PLE options. I am hoping to bury this complicated stuff in these getx functions. I'll start with vanilla BtS and then worry about PLE.

Code:
		# add promo frame
		screen.addDDSGFCAt(sBupString + "PromoFrame", vBupPanel, sFileNamePromo, xPixel + 2, yPixel + 2, 32, 32, WidgetTypes.WIDGET_PLOT_LIST, iIndex, -1, False )
		screen.hide(sBupString + "PromoFrame")

This should be extracted to a create() function. I think here is where you have a great opportunity for abstraction and generalization that will save you a lot of coding. Create a separate object for each element. I'll write more about this tomorrow as it will get pretty complex.
I totally agree with your comments and have noted them down. However, I am 100% sure that I have no idea how to even approach this. My thinking was to get promo frame up and running and then to see if I can extract that do a create() function. I guess I am putting off what I have no idea how to do while I work on something that I have a 25% idea about.
 
Constants are good. One of the most difficult things I found about working with Firaxian CIV files and DungeonSiege were the designers reliance on hardcoded numbers for GUI elements. DungeonSiege even more so as it made the frontend GUI scale extremely poorly with higher than 1024x|1280x resolutions.

Of course at least in DS their .gas (script) files were basically runTime C code, as opposed to having to learn a new language (Python) for CIV.
 
Part of me agrees, but the PLE action is to actually show the 'top' unit at the top left, while the vanilla BtS action is to show the 'top' unit at the bottom right.

I think I would handle that behavior with the sorting stage that creates the list of UnitDisplay objects that are to be drawn on-screen. However, you are spot on to hide this behind the getX/Y() functions.

My thinking was to get promo frame up and running and then to see if I can extract that do a create() function.

That is exactly what I would do first. The first one (promo frame) lets you figure out the basic flow. The second one (e.g. unit icon) should be similar but add a new twist, or perhaps do another one just like promo frame before this one. From these you can then find the commonalities and extract some base functions that don't vary or only vary slightly and build yourself a class to contain the common functions and placeholders for the ones that differ.

I find it far easier to devise an abstraction with a few cases worked out than to do it up front.
 
the "stuck" icons were pretty much just like normal unit icons.. but they weren't tied to a unit, and are always on screen. when you select a stack they get mixed into that stack, and when no stack is selected they are still on screen. selecting them puts a yellow border around them, but they dont do anything beyond that.

dont think i had any filters on at the time.

try it. turn off PLE mid-game and you'll start seeing some units always on the interface.
This looks like the bug that we saw the other day. I've got a solution for this that I will fold into the SVN version of BUG.

Edit: Fixed in revision 1882.
 
Code:
def _drawPromo():
	if self.pCurrUnit.bPromo:
		self._showPromo()

def _updatePromo():
	if self.pCurrUnit.bPromo and not self.pPrevUnit.bPromo:
		self._showPromo()
	elif not self.pCurrUnit.bPromo and self.pCurrUnit.bPromo:
		self._hidePromo()

def _hidePromo():
	if self.pPrevUnit.bPromo:
		self.screen.hide(_getPromoString())

def _showPromo():
	self.screen.show(_getPromoString())
I was working on this a little over the week end. I think some of the above is redundant so I put it in as follows:

Code:
def _updatePromo():
	if not self.pPrevUnit.bPromo:
		self._showPromo()
	elif not self.pCurrUnit.bPromo:
		self._hidePromo()

def _hidePromo():
	if self.pPrevUnit.bPromo:
		self.screen.hide(_getPromoString())

def _showPromo():
	if self.pCurrUnit.bPromo:
		self.screen.show(_getPromoString())
 
The reason for the seemingly duplicate functions is that they each serve a very different need. Below "graphic" means whatever is appropriate for the feature.

Draw - Show the graphic if necessary.

This is called to draw a unit when nothing is visible currently. It is called the first time a stack is selected from none being selected and when a unit appears in a cell that was previously empty (these are really the same case). It must check the unit being displayed to determine whether or not to call show().

Erase - Hide the graphic if necessary.

This is the reverse of draw(). It hides the graphic if it's currently visible. It should check the previous unit to decide whether or not to call hide().

Show - Show the graphic.

The graphic has been determined to be visible now where it wasn't previously.

Hide - Hide the graphic.

Reverse of show(), it hides the graphic that is known to be currently visible. In my function above I tested pPrevUnit, but this should be moved to the new function erase(). It should look like show() which doesn't examine any units.

This function is also called when switching between PLE and non-PLE mode to ensure that all graphics are hidden properly. It would be followed by clearing the "previous units" and calling draw() for each cell.

Update - Show or hide the graphic as needed.

This compares the state of the previous and current units--including testing for None--and calls either hide() or show(). This is the real crux of the speed improvement.

This function also needs to check beyond hide/show for the mission indicator, health and move bars, and dot (GG/moved/injured).
 
ok, fair enough. However ... Q: Should 'update' show or hide or should it draw or erase?
 
ok, fair enough. However ... Q: Should 'update' show or hide or should it draw or erase?

Both. ;) Looking at my psuedocode above, you can see it calls draw(), hide(), and show() under different circumstances. Ideally it will call hide()/show() if it knows for sure that they should be called. In the case of draw(), it calls that because it only knows that there is a unit where none was there before. It leaves it up to draw() to decide whether or not to call show().

Code:
if not pCurrUnit:
    if not pPrevUnit:
        # both have no unit, nothing to do
    else:
        # empty -> full
        draw(pCurrUnit)
else:
    if not pPrevUnit:
        # full -> empty
        hide(pPrevUnit)
    else:
        # both full
        update(pPrevUnit, pCurrUnit)

Now that I'm thinking more about this, I like more the idea of creating a class for each graphic. The graphic class would decide which fields are required from the CyUnit (some take multiple) and produce a key that determines how the graphic is drawn. When no unit is in the cell, it would have a default "none" key that is used.

Its update() function would use the previous and current keys to do the drawing, and it would have the same hide()/show() functions as above and possibly draw()/erase() as well (gotta figure that out). I'll throw some actual code together to see if I can solidify this idea into reality.
 
let me upload what I have - about 45 mins from now.

Edit: or now - committed.
 
As I so often find, everything changes considerably once you start writing actual code. ;) I'm pursuing the idea of having a class for each "artifact" which is typically a single graphical element. This means that the GG and Wounded Indicator options are combined into a single artifact.

Here are the concrete artifacts:

  • Unit Icon
  • Star/Dot
  • Health Bar
  • Move Bar
  • Promotion Frame
  • Upgrade Indicator
  • Mission Tag
I am writing base classes to handle some of the artifacts more generally. For example the Promotion Frame and Upgrade Indicator are both simple toggled graphics. I have also found that we can change the image used on a DDSGFC. I wonder if recreating the DDS each time is part of the slowdown (upgrade, mission tag, star/dot). Probably not--more likely it's just the redraw that causes the delay.

What I'm finding is that all artifacts will need create(), draw(unit), erase(unit), and update(prevUnit, currUnit). The separate hide() and show() functions are specific to the ToggledArtifact class used by the upgrade/promotion indicators.

What I've done is have each artifact have a function to create a key for a unit that encapsulates all of the data needed to draw itself. These keys are then passed to the above functions instead of the unit object. This allows the framework to compare keys and call the appropriate function. For example, if the prev and curr keys are equal, update() is not called for that artifact.

This makes the concrete artifact code much easier to write and should result in the most significant speed improvement.

If you can post your code we can start working toward a common interface between the two.

One thing I noticed is that the upgrade indicator might be responsible for a big time cost. I haven't measured it alone yet, but every unit is checked for possible upgrades every time it is drawn. I'm guessing that this performs a loop over all unit types checking to see if the current unit can upgrade to it, and if so, calculating the cost and comparing it to the player's gold. We could speed this up by keeping a list of all unit types that have upgrade possibilities and updating it each turn (it shouldn't change during a player's turn). This makes the check for upgradability a quick check in a set (near instant runtime). At the same time it would be good to change the indicator to red for possible but cannot afford and green to can afford.
 
bump - EF ... are you around tomorrow (Sat 19th) to discuss this? I am going to look at the maininterface file and add a variable (UsePLEClass) to create a logical branch in the code. 'On' and it uses our new code, 'Off' and it uses the existing code.
 
Top Bottom