Skip to content

Conversation

@LifWirser
Copy link
Contributor

Thank you for submitting a pull request and becoming a contributor to the Vega Strike Core Engine.

Please answer the following:

Code Changes:

Issues:

  • Please list any related issues

Purpose:

  • What is this pull request trying to do?Add support for : Doxygen, Valgrind, GDB (still incomplete

  • What release is this for?All

  • Trubleshootring code documentation

Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to some of this needing to use the automation we've added (authors) or need to add (change log); this also introduces an extra unnecessary layer of directories (doc/doc).

So I'd like to see the doxygen stuff directly under doc, not doc/doc, and things simplified up a bit for easier maintenance. And please keep in mind we're going to add some automation of some of this to be as reliable as we can about it - especially authors.

doc/doc/AUTHORS Outdated
@@ -0,0 +1,51 @@
Vega Strike
http://vegastrike.sourceforge.net
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LifWirser
Copy link
Contributor Author

AUTHORS already exsisted but while I'm here I'll try to delete, I'm trying to move doxygen data to where I had intended

@LifWirser
Copy link
Contributor Author

If I am seing corredtly all should fixed

@stephengtuggy
Copy link
Contributor

This is better. Looks like the files are in their correct directories now. However, I don't know that we need to delete the preexisting AUTHORS file. Then it will just need to be regenerated.

Apart from that, this looks good.

Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please restore doc/AUTHORS for this PR, and let's explore that in its own PR

Replaced /doc/AUTHORS, per request to handle seperate issue not yet entered
stephengtuggy
stephengtuggy previously approved these changes Aug 24, 2022
Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Contributor

@vincele vincele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need significant work to be mergeable, but anyways thanks for the effort.
Be careful with your whitespace, and try to use a spell checker, it should help.


Units are accessed through a UnitContainer or a UnitCollection depending on the number. These structures keep reference counts and remove any "dead" units from them when accessed. Star system has the complete list of units and cycles through them to accomplish physics, etc.

Included in this directory contains resources for generating documentation/degugging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

degugging ?


For more Details please see the doxygen Documentation.
For more Details please see the
doxygen Documentation- {VS_root}/doc/VS_Doxygen.RDME .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Documentation-" ?
remove the dot

@@ -0,0 +1,6 @@
The basic idea is first to Warn you will require lots of space if you proceed.
The basic steps are in the VSE root directory run "doxygen ./doc/doxygen/VS_doxy.Config" and all the data will be in a subdir under /doc/doxygen. the VS_doxy.config is editable (please make a bakup of the config file first, just in case. Currently it is configured to run on VegaStrike github master >"INPUT" needs to be adjusted for appropriate directoy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the added whitespace at start intended ?
"directory run" remove doubled space

The basic idea is first to Warn you will require lots of space if you proceed.
The basic steps are in the VSE root directory run "doxygen ./doc/doxygen/VS_doxy.Config" and all the data will be in a subdir under /doc/doxygen. the VS_doxy.config is editable (please make a bakup of the config file first, just in case. Currently it is configured to run on VegaStrike github master >"INPUT" needs to be adjusted for appropriate directoy.

Requires: doxygen, Graphviz, Vegastrike {engine,assets} source packages. Lots of free storage.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Requires: doxygen" remove doubled space

@@ -0,0 +1,93 @@
In order to properly setup and use gdb with pythonon the "vegastrike-engine", some things need to happen.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "python on" ?

4 edit ".valgrindrc" and comment out (with "#") the "python,python2" lines.
On my system python and python .supp files are the same. so not taking chances I comment them out ,Also the
lines of openmpi-valgrind.supp and pmix/pmix-valgrind.supp are not used in vegastrike last time I checked ,
so they can be commented or given suppression tag as you see fit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace damage, again
what is a suppression tag ?

On my system python and python .supp files are the same. so not taking chances I comment them out ,Also the
lines of openmpi-valgrind.supp and pmix/pmix-valgrind.supp are not used in vegastrike last time I checked ,
so they can be commented or given suppression tag as you see fit.
Also insert "--suppressions=" before all other lines. DON'T FORGET TO SAVE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this may be it...

lines of openmpi-valgrind.supp and pmix/pmix-valgrind.supp are not used in vegastrike last time I checked ,
so they can be commented or given suppression tag as you see fit.
Also insert "--suppressions=" before all other lines. DON'T FORGET TO SAVE
5 run valgrind simular to "valgrind --tool=callgrind ./vegastrike-engine -d../../Assets-Production"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"simular to" => "like this"

5 run valgrind simular to "valgrind --tool=callgrind ./vegastrike-engine -d../../Assets-Production"
in this example callgrind will be run - many other options

I found there is a "gdb server option" but for now this will be a start
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line

@@ -0,0 +1 @@
The "doxyhen.config.orig" was originaly in /engine directory and all reletive files ,edit to run from a directory other than }VS|/engine location (VSE_root)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doxyhen => doxygen
originally in ./engine
relative
whitespace damage (multiple instances)

@vincele
Copy link
Contributor

vincele commented Dec 29, 2022

Also the PR title does not explain what this is about, please fix it

Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good; but please fix @vincele 's comments.

@royfalk
Copy link
Contributor

royfalk commented Oct 2, 2024

What's the status of this PR? It looks like it's 90% done.

@stephengtuggy
Copy link
Contributor

@LifWirser it would be great if you could take a look at fixing the typos @vincele noted. (Also, please merge in the latest changes from master.) Then we could get this PR merged. As @royfalk noted, it's 90% finished. Let's get it across the finish line.

@stephengtuggy stephengtuggy self-requested a review March 14, 2025 20:32
@stephengtuggy stephengtuggy dismissed their stale review March 14, 2025 20:33

I shouldn't have approved this originally. There are a number of typos.

enter this at ">>>" prompt

>>> import os
>>> print('I am pid ()'.format(os.getpid()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
>>> print('I am pid ()'.format(os.getpid()))
>>> print('I am pid {}'.format(os.getpid()))


>>> import os
>>> print('I am pid ()'.format(os.getpid()))
I am pid ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
I am pid ()
I am pid 19612

@royfalk
Copy link
Contributor

royfalk commented Jun 22, 2025

@LifWirser should someone else take over this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants