Jump to content
The Dark Mod Forums

Recommended Posts

I'm sure it's possible, but I'd prefer not to have to re-invent the wheel, at least in DarkRadiant.

The SimpleSockets code in particular looks pretty neat and easy to understand:

    CActiveSocket socket;       // Instantiate active socket object (defaults to TCP).
    char          time[50];

    memset(&time, 0, 50);

    // Initialize our socket object 
    socket.Initialize();

    // Create a connection to the time server so that data can be sent
    // and received.
    if (socket.Open("time-C.timefreq.bldrdoc.gov", 13))
    {
        // Send a request to the server requesting the current time.
        if (socket.Send((const uint8 *)"\n", 1))
        {
            // Receive response from the server.
            socket.Receive(49);
            memcpy(&time, socket.GetData(), 49);
            printf("%s\n", time);

            // Close the connection.
            socket.Close();
        }
    }

 

  • Like 1
Link to post
Share on other sites
  • Replies 88
  • Created
  • Last Reply

Top Posters In This Topic

Top Posters In This Topic

Popular Posts

New video:  

This is how it looks right now. Also attached the code of main class if anyone is interested.   One nuisance is that reloading a map works only after the map is saved. So when mapper moves

I have an idea of adding TDM connection management to DarkRadiant. The plan is to combine it with TDM hot reload features later. Here is a brief list of features: start/connect to TDM, sto

Posted Images

When I use the new mode on large map (BCD), it takes about a second to save map diff.
Map diff usually consists of one or two entities.
I save only them by passing custom "traverse" function.

Profiler shows that MapExporter::prepareScene takes half of the second(the other half is taken by its destructor).
It is clear that the called functions run over the whole scene regardless of the "traverse" filter.

image.thumb.png.78ff761347752812f61f751c1d3b32c8.png

Any ideas?
The latency of one second is not impressive 🤣

  • Haha 1
Link to post
Share on other sites

Sadly DarkRadiant is poorly optimised in a lot of ways. In particular, the scenegraph is actually pretty flat (it's a basically a root with a list of entities underneath, and each entity might have some brushes as children), and a lot of functions will end up traversing the entire scenegraph to find something or perform some operation.

I'm not sure why we pre-iterate over the scene to remove origins and recalculate brush windings on every save, but I'm guessing it was added to fix some bug with out of date data, so I'd be very cautious about trying to remove that code or convert it into on-demand updates, at least without some very extensive testing. If both of those method calls are doing a complete traversal, you could try combining them into a single operation that would traverse the scenegraph once but do both tasks for each primitive it finds. 

To be honest, 1 second to save a large map doesn't sound that bad to me — if it was 20 seconds we would probably have spent some time trying to optimise the process.

I do like that profiler though. Is it Visual Studio? Almost makes me want to try developing on Windows; using perf on Linux is so incredibly primitive compared to the graphical profilers available on other platforms.

Link to post
Share on other sites
18 hours ago, OrbWeaver said:

In particular, the scenegraph is actually pretty flat (it's a basically a root with a list of entities underneath, and each entity might have some brushes as children), and a lot of functions will end up traversing the entire scenegraph to find something or perform some operation.

It's rarely a problem. Traversing 10K objects should take 0.1 ms normally, so I don't think it takes more than 1 ms even if the code is "heavy".

Quote

I'm not sure why we pre-iterate over the scene to remove origins and recalculate brush windings on every save, but I'm guessing it was added to fix some bug with out of date data, so I'd be very cautious about trying to remove that code or convert it into on-demand updates, at least without some very extensive testing. If both of those method calls are doing a complete traversal, you could try combining them into a single operation that would traverse the scenegraph once but do both tasks for each primitive it finds. 

The problem is not that it is doing this, the problem is that it is recomputing all brushes even when I export one entity. For some reason author wanted to push it into constructor, where traverse function is not available yet. It should be moved to MapExporter::exportMap and use traverse function to avoid processing things which are not exported.

Quote

To be honest, 1 second to save a large map doesn't sound that bad to me — if it was 20 seconds we would probably have spent some time trying to optimise the process.

For saving the map it is OK. But I'm trying to replace in-game editing tools. The idea is that mapper enables auto-update mode, selects a light entity, and starts tweaking its parameters. Every change he does should result in immediate update in game. For this purpose one second is annoying.

Quote

I do like that profiler though. Is it Visual Studio? Almost makes me want to try developing on Windows; using perf on Linux is so incredibly primitive compared to the graphical profilers available on other platforms.

Yes, it is built-in profiler. The situation was equally bad on Windows before MSVC 2013 😪

The underlying mechanism is simple: pause process every 1 ms and collect stack trace. I think perf does that equally well. The problem is that nobody cares about displaying information to user in convenient way. Flame charts is well-known way to represent this data statically (you should be able to create it from perf data). But something graphical and interactive is much better of course. The last bit is showing lines in C++ code: this is really hard, since it requires looking deeply into debug symbols, and needs high quality of debug symbols in presence of optimizations (GCC seems to be quite bad at this).

Link to post
Share on other sites
1 hour ago, stgatilov said:

The problem is not that it is doing this, the problem is that it is recomputing all brushes even when I export one entity. For some reason author wanted to push it into constructor, where traverse function is not available yet. It should be moved to MapExporter::exportMap and use traverse function to avoid processing things which are not exported.

"Author" would be me. That brush translation code is an ancient workaround, which was introduced to fix editing behaviour for child brushes of func_* entities (somewhere around 2007 - there were a couple of issues similar to https://bugs.thedarkmod.com/view.php?id=169). The D3 map format needs child brushes to be saved in local entity coordinates, but GtkRadiant/DarkRadiant couldn't handle that correctly. The proper fix would have been a lot of work and probably it would have been above my head back at that time - so the workaround was never removed and carried over since today.

Link to post
Share on other sites
1 hour ago, stgatilov said:

New video:

That's amazing!!

This thread has become very technical, so the average forum user probably stopped reading it. You should post that video also in this referenced thread, to let people know about this cool new feature. 🙂

 

 

  • Like 1
Link to post
Share on other sites

I think I'm ready to start code review.
Should I do a standard fork + push + pull request on GitHub?
 

The known problems on DR side are:

  1. Undoing spawnarg change goes unnoticed (includes moving and rotating stuff). Because such undo does not trigger entity observers. I tried to put some specific observer on undo, but I could not understand where to get entity name from. This is rather serious, but probably not easy to fix, so it can be done later.
  2. When the feature is enabled, closing TheDarkMod in most cases blocks DarkRadiant forever. This is because ZeroMQ ignores closing of the socket. One way to fix it is to replace ZeroMQ with something else. Since ZeroMQ is a third-party dependency, this problem should be fixed before merge.
  3. Relatively slow updates on large maps due to MapExporter constructor processing all brushes. This is not important, and even negligible for small maps. It can be fixed later.
  4. Pretty stupid GUI (because I spent zero time on it). Luckily, @OrbWeaver offered his help for this. There is no point in doing anything about it before merge. I can probably draw a sketch of how I imagined the GUI myself, but my opinion here is pretty weak.

Speaking of ZeroMQ problem. I really like the DFHack/clsocket library, in a sense that it is something very small. Even if we discover any issues, it should be easy to fix them ourselves.
Should I attempt to switch to this library, removing ZeroMQ in process?
And if the answer is "yes", then how should I integrate it into the code? Simply embed C++ code into libs/clsocket?
 

Speaking of testing the changes, a recent trunk of TheDarkMod is required.
I think I will publish a dev build of TDM soon, in which case TDM's SVN won't be necessary.

  • Like 1
Link to post
Share on other sites
6 hours ago, STiFU said:

That's amazing!!

This thread has become very technical, so the average forum user probably stopped reading it. You should post that video also in this referenced thread, to let people know about this cool new feature. 🙂

 

 

I agree, mappers are going to freak out over the ability to test real-time lighting!

  • Like 2
Link to post
Share on other sites
On 8/22/2020 at 11:59 AM, stgatilov said:

I think I'm ready to start code review.
Should I do a standard fork + push + pull request on GitHub?

Yes, that would be simplest I think. Although I am not on GitHub myself (my repo is on GitLab), this is no problem because I can do the merge using the command-line tools while pulling anonymously from any accessible repository.

On 8/22/2020 at 11:59 AM, stgatilov said:

The known problems on DR side are:

  1. Undoing spawnarg change goes unnoticed (includes moving and rotating stuff). Because such undo does not trigger entity observers. I tried to put some specific observer on undo, but I could not understand where to get entity name from. This is rather serious, but probably not easy to fix, so it can be done later.

I think the fix for this would be for entities to properly update their observers when importing a Memento from the undo system, which I'm guessing they don't currently do.

On 8/22/2020 at 11:59 AM, stgatilov said:

Speaking of ZeroMQ problem. I really like the DFHack/clsocket library, in a sense that it is something very small. Even if we discover any issues, it should be easy to fix them ourselves.

Should I attempt to switch to this library, removing ZeroMQ in process?
And if the answer is "yes", then how should I integrate it into the code? Simply embed C++ code into libs/clsocket?

Yes, if you like the clsocket library I suggest switching to it now, before too much code is linked into ZeroMQ (which it sounds like we don't need and doesn't add any real value). I think the idea behind clsocket is that it is really tiny and you could just embed it in the libs directory directly without needing to worry about dependency tracking.

  • Thanks 1
Link to post
Share on other sites

@OrbWeaver, two more questions:

  1. The cpp files of clsocket --- which project should I put then in? There is "lib" project which only has header-only libraries, and four static libraries: mathlib, scenelib, wxutillib, xmlutillib. I have put the cpp files to Radiant project for now, but it does not feel right.
  2. Is there any standard about indentation? Should it be tabs or spaces? I don't see any mention of this on the wiki page.

 

Link to post
Share on other sites

I have created PR: https://github.com/codereader/DarkRadiant/pull/12

I'm using hg-git for GitHub. It seems that I have bumped into a problem with it. Since stupid git does not track file renames, and hg does not attempt to guess them (unlike git), for me the recent refactoring looks like "files deleted here, files added there", without any connection between them 😲 So merging recent master brings conflicts of kind "file modified in A but removed in B". I guess I will leave the branch on old master until review is over.

 

UPDATE: Ok, here are "how-to-test" instructions.

  1. Get the dev16010-8948 version of TDM here or get the most recent trunk of TDM assets repo. Building from source code trunk is OK too.
  2. Get the DR source code with my changes included. Obviously, build it.
  3. Run TDM, start the FM and map that you want to test.
  4. Bring console and make sure com_automation cvar is 1 (enabled).
  5. Run DR and open the FM/map you like to work with.
  6. Now go to "connection" menu and choose some feature there.

The most confusing part is that current GUI does not show the status of game connection. If you forget to set the cvar, or just close TDM accidentally, then DR will silently drop all your requests, and there is no visual cue about it. Obviously, a proper GUI would need a "status light", turning red when connection is lost. The same applies to all "modes" which can be enabled/disabled.

Link to post
Share on other sites
4 hours ago, stgatilov said:

@OrbWeaver, two more questions:

  1. The cpp files of clsocket --- which project should I put then in? There is "lib" project which only has header-only libraries, and four static libraries: mathlib, scenelib, wxutillib, xmlutillib. I have put the cpp files to Radiant project for now, but it does not feel right.

 

If it's only used by code in radiant, putting it in there is OK, but if you would rather put it in lib (or it might be needed elsewhere), you could create a new subdirectory in lib with both header and cpp files, much like wxutil. Of course that will  need changes to the build scripts to build the new library and link it where appropriate.

Quote
  1. Is there any standard about indentation? Should it be tabs or spaces? I don't see any mention of this on the wiki page.

Typically I use 4 spaces with Allman style braces, although we have a lot of legacy code which uses different styles so you will see a fair amount of inconsistency.

Note that I do strongly prefer function open braces in column 0 where possible, e.g.

void GameConnection::waitAction()
{
    while (_seqnoInProgress)
        think();
}

because Vim has a convenient shortcut to jump to the beginning of a function which only works when the code is formatted like this.

Link to post
Share on other sites
2 hours ago, OrbWeaver said:

If it's only used by code in radiant, putting it in there is OK, but if you would rather put it in lib (or it might be needed elsewhere), you could create a new subdirectory in lib with both header and cpp files, much like wxutil. Of course that will  need changes to the build scripts to build the new library and link it where appropriate.

It is located in lib directory. But it is in Radiant project.
That's about VC projects actually: I did not look yet how it is built with make, maybe there is the same question there.

Quote

Typically I use 4 spaces

I will reformat with spaces (UPDATE: done)
Having mix of spaces and tabs is absolutely pointless.

Quote

with Allman style braces,

With such style I start feeling that I cannot write anything more complicated than a simple getter and still have it all visible on a single screen 🤣
Ironically, it is important, because RAM inside human brain is pretty limited.

Quote

Note that I do strongly prefer function open braces in column 0 where possible, e.g.
because Vim has a convenient shortcut to jump to the beginning of a function which only works when the code is formatted like this.

Well, why not.

Link to post
Share on other sites
35 minutes ago, stgatilov said:

It is located in lib directory. But it is in Radiant project.
That's about VC projects actually: I did not look yet how it is built with make, maybe there is the same question there.

Ah right, that's definitely very non-standard and potentially confusing. If there are CPP files to be compiled inside a subdirectory of lib, this should be a separate sub-project which generates a dynamic library which is then linked into Radiant or whatever else uses it, rather than other projects directly compiling CPP files from inside lib. Alternatively, if you don't want the hassle of creating a library subproject, you could put the CPP files directly inside a subdirectory of radiant at which point they can be compiled directly into the radiant project.

35 minutes ago, stgatilov said:

I will reformat with spaces (UPDATE: done)
Having mix of spaces and tabs is absolutely pointless.

Indeed. I've updated the coding standards page to mention this.

35 minutes ago, stgatilov said:

With such style I start feeling that I cannot write anything more complicated than a simple getter and still have it all visible on a single screen 🤣
Ironically, it is important, because RAM inside human brain is pretty limited.

Note that this is only for function bodies — you are free to use K&R style which allows for opening braces on the same line for for and if statements if you want to save a few lines. I've also mentioned this on the coding standards page.

Link to post
Share on other sites
59 minutes ago, OrbWeaver said:

Ah right, that's definitely very non-standard and potentially confusing. If there are CPP files to be compiled inside a subdirectory of lib, this should be a separate sub-project which generates a dynamic library which is then linked into Radiant or whatever else uses it, rather than other projects directly compiling CPP files from inside lib. Alternatively, if you don't want the hassle of creating a library subproject, you could put the CPP files directly inside a subdirectory of radiant at which point they can be compiled directly into the radiant project.

The problem is that the code itself does not support exporting.
In fact, someone has recently merged dllexport thing into there, but it is weird in many ways, and they know about it. So I took previous version before that.
For dynamic library, there is a choice between waiting for clsocket guys to sort it out or do it ourselves. Hmm... maybe make PR to clsocket? 🤔

I can create separate project as static library. It can be converted to dynamic library later.

Link to post
Share on other sites

I'll have a look at the PR in the next few days, but I can't promise anything. I'll take the liberty and change a few things here and there, and I'm probably going to convert this to a TDM plugin like the conversation editor and the rest.

Link to post
Share on other sites
On 8/27/2020 at 5:01 AM, greebo said:

I'll have a look at the PR in the next few days, but I can't promise anything. I'll take the liberty and change a few things here and there, and I'm probably going to convert this to a TDM plugin like the conversation editor and the rest.

I've merged stgatilov's PR into a topic branch in my repo called topic/gameconnection with some initial Linux fixes, so as and when you do want to have a look at this, feel free to pull from this branch.

Most of the fixes are trivial stuff like #include filename capitalisation and the odd missing standard library header, adding the new files to the Linux Makefile etc. More notable changes:

  • The clsocket library files are now all in a subdirectory of radiant/gameconnection to eliminate the out-of-tree dependency on files inside lib. This mirrors the layout of the picomodel library which is only used by a single set of files under radiant, and would allow the whole thing to be moved into a separate plugin (which I support) at a later time.
  • There is now a proper IGameConnection interface that exposes methods called elsewhere in the code. The first implementation had an empty interface then a dynamic_cast down to the implementing class in the calling code.

I have confirmed that the result builds on Linux and runs, and there is a Connection menu with the expected list of entries. I have not done any actual testing of the functionality at this point.

Link to post
Share on other sites

I clearly don't visit this sub-forum often enough. This is such an awesome feature! Thanks & cheers all around!

What do you see when you turn out the light? I can't tell you but I know that it's mine.

Link to post
Share on other sites
7 hours ago, OrbWeaver said:

There is now a proper IGameConnection interface that exposes methods called elsewhere in the code. The first implementation had an empty interface then a dynamic_cast down to the implementing class in the calling code.

Copying methods to interface made no sense to me, since they were only used once for attaching GUI.
But if you intend to move it to separate module, then it is indeed necessary.

However, if GUI is rewritten to spawn separate window, then the methods which are "public" now can once again be made private to the module. I guess the window will be located in the module too, and only the methods to start/close this window would be exposed from module.

Link to post
Share on other sites
  • 3 weeks later...

I merged your PR into my own topic branch for testing on Linux, but I haven't merged it yet into my master branch because Greebo mentioned in his unit testing thread that he is still planning to merge my master branch into his own repo, and I was worried that integrating your PR first might make this more difficult. Although it probably doesn't affect the difficulty that much (in terms of conflicts etc) whether he merges my master followed by your changes (in my topic branch) as two separate steps, or if I merge it into my master and he then merges my master into his repo in a single step.

So I guess there's no reason I can't merge your changes into my master branch, although note that this doesn't in itself create a Windows build that other people can use, because Greebo makes all the Windows builds by hand and they are built off his repo, not mine.

TL;DR summary: I'm happy to merge your changes into my source this week and I guess that they will make it into the next official DR release if there aren't any major problems, but we don't currently have any predicted release date.

  • Like 1
Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


×
×
  • Create New...