Valyria Tear Code Import Notes

For discussion of the code running behind the game

Moderator: Staff

User avatar
Roots
Dictator
Posts: 8665
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Valyria Tear Code Import Notes

Postby Roots » Fri Dec 07, 2012 6:40 pm

As I am working to import the code changes from VT, I'm having several doubts about some of the major changes that were made. In this thread I'd like to list what those questionable changes are so that we can consider whether we want to approve them, reject them, or approve them with changes. I'll maintain this top post with the list of changes under review and update them as we make a decision on any particular issue. View the provided links to see the commit message and the list of changes made.



Fixed support for fade in/out in the audio manager.
https://github.com/Bertram25/ValyriaTea ... 3392e9f6a8
I was unaware that fade support was not working. My main issue here is that all of the audio effect processing was put in the AudioDescriptor classes, and I'm not sure if that's a good idea or not. In general I am weary about the implementation details here. I think parts of it can be approved with some modifications, but I'm not comfortable adding the feature as-is right now.

Added support for game mode auto unloading
https://github.com/Bertram25/ValyriaTea ... ad83ad31ca
This feature added some desirable changes (remove requirement for pre-loading audio in maps; permitting music used across two maps to keep playing as the map transitions), but I'm not sure if this is the way to do it. The audio code should already be internally using a referencing system that knows when no one is no longer using a particular sound or music file and automatically frees the resource, so I don't see why we need to be passing pointers to GameMode objects here.

Restart a sound if it was played while already playing
https://github.com/Bertram25/ValyriaTea ... f16f5abc82
A simple change. I don't think this is desirable because we may want a sound to play multiple times simultaneously (ie, an explosion sound played several times over during a scene).

Ported the engine to use SDL_Image instead of a plain libpng library.
https://github.com/Bertram25/ValyriaTea ... 385cf94a01
I see absolutely no need for adding a new library dependency so that we could (potentially) support more image formats. Plus I'm very hesitant to be changing this type of core engine logic which has been well tested. I'd rather not burden ourselves with more work that will result in zero benefit for us, as we have no intention of using images other than png and jpg.

Turned the welcome window into a help window.
https://github.com/Bertram25/ValyriaTea ... bf4608dca2
This is a pretty handy feature actually. Pressing F1 will bring up a help window that displays information relevant to the currently active game mode. The problem with this change is that it makes the engine code include code that is found in src/modes, which we should never allow. I do think this is a good feature to have though, so perhaps we can use our own implementation and find a way to do it without this dependency issue.

Fixed light ambiant effects.
https://github.com/Bertram25/ValyriaTea ... 9981bb855f
Definitely a great feature to have. But a second Draw() call was added to all GameMode classes that essentially just breaks up the draw code for a mode into two different calls. The second draw call is intended for things like GUIs, where we don't want any graphical effects to affect those images. This is really unnecessary, because you can just order your single draw call properly. It doesn't do anything but make the code more confusing. So I approve the feature, but will not include the second draw call.

Moved the fade in fadeout management between game modes into the mode manager.
https://github.com/Bertram25/ValyriaTea ... b530ed1f60
I don't like this change because it removes the flexibility for the game modes to control their fading sequences. We want might some fades to be faster, longer, or done slightly differently depending on the action in the game.

Made the ambient, light and lightning effect handled per game mode.
https://github.com/Bertram25/ValyriaTea ... acc4dcaf70
Basically, several graphical features were removed from the video engine to the mode management code. I strongly disapprove of this. The explanation given was to make the video engine smaller by moving this code elsewhere, but that's not a good reason IMO. Graphics code should stick in the graphics engine. Period.

Made the particle management be done per game mode.
https://github.com/Bertram25/ValyriaTea ... 047e7cbee2
Same idea as above. Moved graphics code out of the video engine. Disapprove.

Created a script supervisor as a game mode component.
https://github.com/Bertram25/ValyriaTea ... fa917be9f9
This is a new feature added that basically added a class for each game object to manage it's scripts. It seems to be added as a replacement for the battle mode script code. I'm not sure what to think of this one. I think there may be some good parts, like making it easier to add/draw images in a script. But I also think there are some bad parts, where it tries to be generic enough that it can be everything to every game mode. I'll need to study and understand this more before I have a firm opinion.

Particle effect updates
https://github.com/Bertram25/ValyriaTea ... 7939f4f31e
https://github.com/Bertram25/ValyriaTea ... 596506e48b
https://github.com/Bertram25/ValyriaTea ... 6f39d938ea
I'm skipping over these two commits because they are rather large and I don't really want to spend the time importing and then testing all of this, especially when we do not have an immediate need for this technology. Plus the I feel the general design of all the particle classes is really sloppy and could use a complete overhaul, rather than adding on even additional classes here. Finally, I noticed some particle code going directly into the map code and I'm a little cautious about that. I might just put this on the roadmap as an optional task for now.

Optimizing GL Calls
https://github.com/Bertram25/ValyriaTea ... 485bf6afb1
Bertram made several optimizations by wrapping GL calls to use them only if necessary. This required adding a ton of booleans to the VideoEngine class. While I understand the reasoning here, I wonder if the booleans are really necessary (can't you simply query GL states to see if they are already set?). And if they are, I'd prefer this kind of code to be wrapped in some sort of utility class in the video engine to keep that huge class from getting any more bloated. Plus, optimizations are not really important right now to be spending our time on. The game runs plenty fast already, and I'd prefer to make the video engine code more complete and mature before doing any serious optimization efforts on it.
Last edited by Roots on Wed Dec 19, 2012 8:55 am, edited 4 times in total.
Reason: Added more issues
Image
User avatar
Roots
Dictator
Posts: 8665
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Valyria Tear Code Import Notes

Postby Roots » Fri Dec 07, 2012 11:32 pm

Roots wrote:Fixed light ambiant effects.
https://github.com/Bertram25/ValyriaTea ... 9981bb855f
Definitely a great feature to have. But a second Draw() call was added to all GameMode classes that essentially just breaks up the draw code for a mode into two different calls. The second draw call is intended for things like GUIs, where we don't want any graphical effects to affect those images. This is really unnecessary, because you can just order your single draw call properly. It doesn't do anything but make the code more confusing. So I approve the feature, but will not include the second draw call.


After further examination, I was wrong about it not being necessary. This is because the Display() method in the video engine currently looks like this:

Code: Select all

void VideoEngine::Display(uint32 frame_time) {
   // Update all particle effects
   _particle_manager.Update(frame_time);

   // Update shaking effect
   PushState();
   SetCoordSys(0, 1024, 0, 768);
   _UpdateShake(frame_time);

   // Update lightning timer
   _lightning_current_time += frame_time;

   if (_lightning_current_time > _lightning_end_time)
      _lightning_active = false;

   // Draw a screen overlay if we are in the process of fading fading
   if (_screen_fader.ShouldUseFadeOverlay()) {
      Color fade_color = _screen_fader.GetFadeOverlayColor();
      StillImage fade_overlay;
      fade_overlay.SetColor(fade_color);
      fade_overlay.Load("", 1024.0f, 768.0f);
      SetDrawFlags(VIDEO_X_LEFT, VIDEO_Y_BOTTOM, 0);
      PushState();
      Move(0, 0);
      fade_overlay.Draw();
      PopState();
   }

   // This must be called before DrawFPS, because we only want to count
   // texture switches related to the game's normal operation, not the
   // ones used to draw the video engine debugging text
   if (_advanced_display)
      _DEBUG_ShowAdvancedStats();

   if (TextureManager->debug_current_sheet >= 0)
      TextureManager->DEBUG_ShowTexSheet();

   DrawFPS(frame_time); // Draw FPS Counter If We Need To
      
   PopState();

   SDL_GL_SwapBuffers();

   _screen_fader.Update(frame_time);

   // Update animation timers
   int32 old_frame_index = _animation_counter / VIDEO_ANIMATION_FRAME_PERIOD;
   _animation_counter += frame_time;
   int32 current_frame_index = _animation_counter / VIDEO_ANIMATION_FRAME_PERIOD;
   _current_frame_diff = current_frame_index - old_frame_index;
} // void VideoEngine::Display(uint32 frame_time)


As you can see, many graphical effects are processed here, which is why Bertram added the second draw call so that you could draw elements to the screen that were not affected. However, I still don't necessarily like the implementation that requires two draw calls. Instead, I think it would be better if the modes that use visual effects make a call to the video engine like "ApplyEffects(uint32 time)" that will apply all graphical effects. Then any additional draw calls can be made after the effects are applied and they won't be affected.

I think I'm going to go ahead with this design. It allows the modes to have a little more control over how and when effects are applied. I think it would be worthwhile to make individual effects applicable as well (ie, first apply scene lighting, then particles, then fog), but the ApplyEffects() method would be an easy short-hand for the common case of applying all active effects. My only concern (and this applies to both the two draw call solution and the ApplyEffects solution) is with timing. I'm afraid of the pre and post effect drawing receiving different update times and gradually getting more and more out of sync. But it might not be a problem. We'll just have to see.
Image
User avatar
Bertram
Senior Member
Posts: 127
Joined: Fri Feb 26, 2010 10:08 am

Re: Valyria Tear Code Import Notes

Postby Bertram » Mon Dec 10, 2012 11:36 am

Hi,

Let's discuss a few points I saw in this thread:

Turned the welcome window into a help window.

That one is bothering me as well, and I see this as some unfinished stuff, hence the issue
about it in the VT issue list.
Plus, I'd like to have it configurable, and look much more appealing.

Made the ambient, light and lightning effect handled per game mode.

I won't discuss the "I disapprove" part here, but yes, there is indeed something to finish/redo here.

My own vision of it is that the fading between modes should prevail over game/story fadings/color modulations.
I admit the how to do it is not yet fine in VT, and will have to be fixed eventually, but after the first release considering my own priorities.

Created a script supervisor as a game mode component.


It seems to be added as a replacement for the battle mode script code.

No, it is not. It is (ab)used to give battle dialogues support for now. The script supervisor whatever his name is, is more here
for reusable battle custom animations.
The battle events/dialogues, and AI scripts will indeed need a brand-new design.
User avatar
Roots
Dictator
Posts: 8665
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Valyria Tear Code Import Notes

Postby Roots » Thu Dec 13, 2012 9:13 am

https://github.com/Bertram25/ValyriaTea ... 2feff86e06

In this issue Bertram created a feature whereby an animation could be loaded by a definition stored in a Lua file, which describes the image containing the animation, how to divide up the frames, and the timings of those frames. I certainly like this idea very much, but I did not include this particular aspect in the code import because it requires that each animation definition be contained within its own file. I don't like the idea of needing to keep hundreds, if not thousands of these files over the course of the project. I also don't like that Bertram stored the scripts in the img/ directory, which was meant to contain only image files and nothing more.

I'm eventually going to work this in at a later time, but I'm going to find a place for animation definitions to be stored in dat/, and I'm going to create a few different animation files that store a group of related animations. For example, character_animations.lua could contain all animations for characters, and we could have files for enemy sprites, map objects, etc.

EDIT:

https://github.com/Bertram25/ValyriaTea ... 94a5629ae4
This is a related feature where support was included to allow the script to define an arbitrary sequence of frames and timings. We will definitely need to support custom animation sequences like this, whatever our implementation may be.
Image
User avatar
Roots
Dictator
Posts: 8665
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Valyria Tear Code Import Notes

Postby Roots » Tue Dec 18, 2012 7:18 pm

https://github.com/Bertram25/ValyriaTea ... 717a40cd35

Bertram added this feature to be able to specify texture smoothing on each individual image instead of just relying on the global option to smooth all textures. Then he applied smoothing to only map tiles. I am not convinced that this is necessary, but it is something to keep in mind. Maybe if someone with a strong graphics background comes along, he or she can help us figure out whether this feature is a must-have or not.
Image
User avatar
Roots
Dictator
Posts: 8665
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Valyria Tear Code Import Notes

Postby Roots » Tue Dec 18, 2012 7:45 pm

https://github.com/Bertram25/ValyriaTea ... dd1df95c1a

Bertram added in support for lights here, aa feature of the video engine that has been broken for a long time. But it was added to the map mode code, and not the video engine. I really want this available in the engine so that other modes besides just maps can have this feature available to use. It also appears that additional parameters were set to allow more control over the type of light produced (a good thing). I won't be doing this now, but at some point we should add lights back into the engine and use this implementation as our reference.
Image
User avatar
Bertram
Senior Member
Posts: 127
Joined: Fri Feb 26, 2010 10:08 am

Re: Valyria Tear Code Import Notes

Postby Bertram » Wed Dec 19, 2012 11:07 am

Bertram added this feature to be able to specify texture smoothing on each individual image instead of just relying on the global option to smooth all textures. Then he applied smoothing to only map tiles.

Why?
For two reasons, I was first trying to figure out the tile offset bug along with qubodup at the time, and he used this to try different settings on his own. Without success at the time, but yet, at least we tried.
Secondly, I don't smooth the tiles when playing on my TV (TV + a good usb game pad and it's rocks :approve: ) as I find it looking better. That's why I kept it.
User avatar
Roots
Dictator
Posts: 8665
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Valyria Tear Code Import Notes

Postby Roots » Wed Dec 19, 2012 9:22 pm

I had a feeling that was the reason why you did that. I'm not saying it's a bad feature (quite the opposite, really). It would just take quite a lot of time for me to import that change. I'm getting a little lazy/impatient as importing all of these changes takes a lot of time (I'm really impressed at how much work you did almost entirely by yourself). So some of the changes that are a lot of work but not that critical like this one I'm pretty much leaving up to review/implement at a later time.
Image
User avatar
Roots
Dictator
Posts: 8665
Joined: Wed Jun 16, 2004 6:07 pm
Location: Austin TX
Contact:

Re: Valyria Tear Code Import Notes

Postby Roots » Thu Dec 27, 2012 2:14 am

Script the Opening Animation to Boot Mode
https://github.com/Bertram25/ValyriaTea ... 414a272ae8
I like this very much, but I'm skipping over it because it's not a priority right now and isn't going to make any improvement as far as the user is concerned. Hope we get around to doing it some day.

Reload Text of Boot Menus When Language is Switched
https://github.com/Bertram25/ValyriaTea ... 6c9df977d9
Another change I definitely like, but I'm skipping over for now. I would like to investigate whether or not we can add something into the GUI code itself to reload all text and fetch the text for the currently selected language. That way we wouldn't have to have all of this extra code in boot mode (or any other mode) to re-load the text.
Image

Return to “Programming”

Who is online

Users browsing this forum: No registered users and 6 guests