Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(code): Add HUD display for Solar Power and Solar Wind #164

Open
wants to merge 25 commits into
base: experimental
Choose a base branch
from

Conversation

MadisonianX
Copy link

@MadisonianX MadisonianX commented Dec 2, 2024

Feature

Hi, this is my first PR and I'm new to both coding and using GitHub so please forgive any faux pas; I have been observing the project for a couple years now though and I feel like I'm getting the hang of it enough to contribute.

Summary

In keeping with Delta's philosophy of allowing additional information to be accessed by the player, adds an attribute for "solar power sensor" and "solar wind sensor" to determine the total solar power or solar wind of a system and display it on the HUD (underneath the Navigation panel next to the system minimap).

Very useful for determining which systems—especially binaries—are best for solar collection or ramscoop usage. Or conversely, which supergiants are more productive than binaries with small stars.

Screenshots

Sol:
Screenshot 2024-12-02 at 7 35 04 AM

Alpheratz (binary):
Screenshot 2024-12-02 at 7 32 00 AM

Atik (trinary):
Screenshot 2024-12-02 at 7 29 42 AM

Usage examples

outfit "Astronomical Scanner"
	category "Systems"
	cost 18000
	thumbnail "outfit/astronomical scanner"
	"mass" 2
	"outfit space" -2
	"energy consumption" .1
	"solar power sensor" 1
	"solar wind sensor" 1
	description "	An astronomical scanner provides data on the system's total solar power and wind output, allowing captains to see which systems are best for solar collection or ramscoop usage."

Separate stats introduced for modularity, could easily be a single attribute though.

Testing Done

Purchased an astronomical scanner. Solar Power and Solar Wind display on HUD at 100x the star's respective stats. Binary and trinary systems display combined stats of all stars.

Artwork Checklist

(If any artwork was added or changed by this PR, the following must be provided.)

  • I decline to claim copyright of any assets produced or modified

Adds an attribute for `"solar scan"` and `"ramscoop scan"` to determine the total solar power or solar wind of a system and display it on the HUD.
Copy link

@TheGiraffe3 TheGiraffe3 left a comment

Choose a reason for hiding this comment

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

Wonderful idea!
I do notice you didn't add tooltips for the new attributes.
Also, the solar power generation symbol is already on the other side of the screen. Is it a good idea to have duplicated information?

data/human/outfits.txt Outdated Show resolved Hide resolved
source/Engine.cpp Outdated Show resolved Hide resolved
@TheGiraffe3
Copy link

And by the way, if you haven't noticed already, the PRs to assets and high dpi aren't necessary; Delta doesn't have those repositories.
I almost wonder if the links should be removed altogether.

@@ -258,7 +258,8 @@ outfitter "Common Outfits"
"Pilot's License"
"Laser Rifle"
"Fuel Bar Fixed Display"

"Astronomical Scanner"

Choose a reason for hiding this comment

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

Maybe you'd want to move this to a Deep Sky outfitter; it makes sense for this outfit to be an "improved ramscoop", and that would mean it should be sold alongside the Ramscoop, not separately.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I put it there as a draft for easier testing purposes and figured someone like yourself would have a better recommendation for where it should go permanently if implemented. I have plans to add more to that particular scanner as I get better at coding.

@Zitchas
Copy link
Member

Zitchas commented Dec 2, 2024

Thank you for this PR, I am always a fan of making additional information available to the player.

At first glance I thought this was duplicating the existing information (Flagship's solar power and fuel ramscoop production), but it's not, is it? It's actually displaying the specific solar power and solar wind strengths of this particular system, not calculating power production. So that seems like useful information to have, especially with the display tied to attributes so the player can choose whether or not they make use of it.

I don't have time right now to go in-depth into this, but I like the concept and the screenshots look good at first glance. It might be a few days before I can provide more than surface level commentary, though. Just wanted to give you a headsup on that so I don't give the impression of ignoring it or anything.

Anyway, thanks! I look forward to digging into it more.

edit:

Hi, this is my first PR and I'm new to both coding and using GitHub so please forgive any faux pas; I have been observing the project for a couple years now though and I feel like I'm getting the hang of it enough to contribute.

Hi! And don't worry about GitHub etiquette. Being polite and positive are the most important things. So long as those are maintained, we should be good. :)

@Zitchas Zitchas added enhancement New feature or request code Things that focus on the mechanics and internals of how they have engine works. labels Dec 2, 2024
@MadisonianX
Copy link
Author

@TheGiraffe3 I borrowed the same solar power icon but could come up with something different if necessary. My goal was to introduce another PR that alters the flagship solar collection & ramscoop displays to be numerical above the respective meters (and add solar heat as well) so no icons would be needed for those, as I felt the vertical list was getting a little crowded.

Screenshot 2024-12-02 at 8 25 47 AM

Was just following the PR template, I'll skip the repo parts next time.

I have tooltips but forgot to include them. I'll have to figure out how to edit the PR later on.

@Zitchas You're correct, it's the total solar power and solar wind of the system, unaffected by ship distance and separate from the flagship solar collection and flagship ramscoop. The star map is aesthetically pleasing but difficult for determining solar power/ramscoop strength given the range of star powers, especially when combined in binary/trinary systems; this gives that info when present in the system at least. Goal being to also add that info to the Map Panel in some manner.

I've been lurking for 3-4 years now so I know how long it takes which is why I'm focusing on the more rapid experimental branch, but even so I'm not in any rush for you to review. I'm learning C++ by dissecting the code here so I just wanted to get started with something simple; as I get more familiar with GitHub I hope to drop a flurry of ideas for everyone to look at.

@MadisonianX
Copy link
Author

Moved scanner to Deep Sky Basics and Deep Sky Advanced, added tooltips.

@TheGiraffe3
Copy link

@TheGiraffe3 I borrowed the same solar power icon but could come up with something different if necessary. My goal was to introduce another PR that alters the flagship solar collection & ramscoop displays to be numerical above the respective meters (and add solar heat as well) so no icons would be needed for those, as I felt the vertical list was getting a little crowded.

Alright, makes sense. I was just worried that the numbers would be the same, but you're right, they aren't.

I've been lurking for 3-4 years now so I know how long it takes which is why I'm focusing on the more rapid experimental branch, but even so I'm not in any rush for you to review. I'm learning C++ by dissecting the code here so I just wanted to get started with something simple; as I get more familiar with GitHub I hope to drop a flurry of ideas for everyone to look at.

That's pretty much what I'm doing :).


(and by the way, you needn't worry about the commit titles; all ES pull requests are squashed and make one commit, so there's no need to have sensible commit names)

`Allows scanning of stellar objects to determine their precise power output for display on the HUD.`

tip "ramscoop scan:"
`Allows scanning of stellar objects to determine their precise wind output for display on the HUD.`

Choose a reason for hiding this comment

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

Suggested change
`Allows scanning of stellar objects to determine their precise wind output for display on the HUD.`
`Allows scanning of stellar objects to determine their precise solar wind output for display on the HUD.`

@ravenshining
Copy link
Member

Having the readout on the left seems to imply that that wind and sun a property of your targeted ship, not your current star system. I would put this info at the extreme top right, to the left of the "current system" box, so it's clear what the information is associated with.

You could even have a second display on the top left, showing the solar and wind available in the system you've selected to jump to

@Zitchas
Copy link
Member

Zitchas commented Dec 3, 2024

It'd take a bit more artwork (resizing the box and all), but having the solar and solar wind strength for the destination system inside that box with the destination system (if known, at least) would be very useful.

So long as the solar power and solar wind icon & text are positioned via the interfaces.txt file, I'm not super concerned about its position, as that can easily be moved later (or even by individual players).

That being said, I feel that next to the Radar seems reasonable that it's info about the system. I'd be concerned if it was lower down by the targetting reticule. That being said, our "Current System" listing is top right, and being as close to that as possible would make sense... But that very top right area is already kind of full, and getting it in too close to the flagship info would risk it being misinterpreted as part of the flagship's stats.

Hmmm... I wonder about moving the entire HUD down a line in order to expand the current system box a bit so it could have a solar power and solar wind icon right below it, if people have the requisite equipment, that is. That's probably a fair bit of work, though.;

(No pressure: I'm 100% OK with this being just on the left side about where you have it now, and then doing follow up PRs if you are interested in implementing the destination stats or moving it from the left side into the current system box idea.)

@MadisonianX
Copy link
Author

MadisonianX commented Dec 3, 2024

Having the readout on the left seems to imply that that wind and sun a property of your targeted ship, not your current star system. I would put this info at the extreme top right, to the left of the "current system" box, so it's clear what the information is associated with.

I had tested it to the left of the system/date/credits box at first but it just didn't seem natural. When you jump into a system, you tend to look at the minimap first to assess the situation. It's well above the target ship info, is present before any ship is targeted and does not change when de-targeting ships, and I think the two areas are fairly well delineated as either system or target (admittedly I moved my target section slightly lower).
system info

As far as the UI, I honestly think the flagship name should go where the system name is now (the flagship area), and the system name should be moved to the top center with a smaller version of the Map Panel's "currently selected system" banner. I'll open a PR with that next, looks better in testing.

You could even have a second display on the top left, showing the solar and wind available in the system you've selected to jump to

&

It'd take a bit more artwork (resizing the box and all), but having the solar and solar wind strength for the destination system inside that box with the destination system (if known, at least) would be very useful.

That could probably done in an additional PR if I study the code more, would have to figure out how to call the data for system that's selected for hyperspace jump. I'd prefer to have that data available on the map itself but the ideas aren't mutually exclusive.

Copy link
Member

@Zitchas Zitchas 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! I have a few comments and suggestions, but that aside, I very much like this idea as it gives pilots the tools to learn more about their environment.

I know in #166 We are discussing re-arranging a few elements of the UI to better present system information. I would anticipate that we merge this (164) in fairly short order as it looks pretty good with just some minor changes; and then polishing off the location & presentation of the info in the UI in 166.

While most of my suggestions are just that: Suggestions, listing the new images in the copyright is a requirement. Note that you don't have to use your real name there if you do not wish to, but it should be something that is connected to you, such as your GitHub user name. If the images are sourced from somewhere else, then the source's handle or online name are acceptable as well, along with whatever copyright license they make the images available under.

data/human/sales.txt Outdated Show resolved Hide resolved
@@ -349,6 +351,7 @@ outfitter "Deep Sky Advanced"
"In-flight Mass Display"
"Hyperjump Fuel Display"
"Fuel Bar Hyperjump Display"
"Astronomical Scanner"
Copy link
Member

Choose a reason for hiding this comment

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

These are a good place for this outfit for now, although in the future it might be more interesting to have it in its own outfitter and have it only available from a few worlds that mention having universities or astronomy studies, and tie it into some research missions from said places.

Copy link
Author

Choose a reason for hiding this comment

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

I have it in Deep Sky Basics and Deep Sky Advanced currently, is just Advanced preferred?

Copy link
Member

Choose a reason for hiding this comment

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

Probably just advanced, I think.

Copy link
Member

Choose a reason for hiding this comment

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

Nice artwork!

Don't forget to put an entry in the copyright file labelling where it comes from. It would also be appreciated to have a copy of the image that is double the size along side it, with @2x appended on the end of the filename.

Copy link
Author

@MadisonianX MadisonianX Dec 14, 2024

Choose a reason for hiding this comment

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

The scanner image is just a placeholder, I'm currently working on something original. The wind icon was just a generic free stock image I don't even remember from where, if it even matters?

Should @2x typically be added with each PR that has an image, or to a central @2x database like the main branch does?

Does it matter where in the copyright file things should go or just add info at the end?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please include the 2x here. We don't have a secondary high dpi location currently.

There isn't much of an order to the copyright, just so long as it is in the art section, and people go before organizations.

source/Engine.cpp Outdated Show resolved Hide resolved
}
// Display combined Solar Wind for system
int displaySystemWind = (flagship->DisplaySystemWind() * 100); // Multiplied to better display decimal inputs
if(displaySystemWind >= 0.01 && flagship->Attributes().Get("ramscoop scan"))
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be less confusing to call this "wind scan" or perhaps "solar wind scan"; although in following with my comment about the solar scan, maybe "wind sensor" or "solar wind sensor"?

Copy link
Author

Choose a reason for hiding this comment

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

Sensor is fine, preferably "solar power sensor" and "solar wind sensor" so it's precise in what attribute each is sensing. Or if making them separate attributes is overkill then just "solar sensor"; I was just following the example of the tactical and strategic scanner modularity.

Copy link
Member

Choose a reason for hiding this comment

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

I personally like having them as separate values. :) could be good to have some fraction that just tracks, say, solar power.

@Zitchas
Copy link
Member

Zitchas commented Dec 11, 2024

OK, the relocation of the system name to above the minimap has been merged. So that gives a slightly different UI layout to work with here.

Copy link
Member

@Zitchas Zitchas left a comment

Choose a reason for hiding this comment

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

Just flew around with it a bit, and looks good. I've got two suggestions that just tweak the position of the icons ever so slightly. Those are basically just splitting hairs.

The code looks good.

The actual "required" change is adding a listing to the copyright file for the solarwind image. Once that's in, I think this is ready to merge.

data/_ui/interfaces.txt Outdated Show resolved Hide resolved
data/_ui/interfaces.txt Outdated Show resolved Hide resolved
data/_ui/interfaces.txt Outdated Show resolved Hide resolved
MadisonianX and others added 5 commits December 24, 2024 12:04
Please correct as necessary. All the stuff in the comments should be removed if you entirely created both, or with a "derived from works by X" if that is the case.
@Zitchas
Copy link
Member

Zitchas commented Jan 2, 2025

OK, I figured out how to push a change directly to the PR, so I added a copyright section for you, along the lines of:

Files:
 images/outfit/astronomical?scanner*
 images/ui/tactical/wind.png
Copyright: MadisonianX (https://github.com/MadisonianX)
License: CC-BY-SA-4.0
Comment: Unclear if these are derived from anything or entirely original. If one is derived and the other original,
 then this copyright entry can be split to have one for the one, and a second for the other.
 (If unclear, derived means a pre-existing image was modified to get this)

Obviously, I do not actually know the provenance of the imagery, nor do I know what name you would like attached to it. You are welcome to just use your GitHub handle like I put here, using another name and an email address is good too.

If the two images are from different sources; for instance the wind image you created entirely yourself, while the astronomical one is based on (derived from) an image someone else made, then this entry can be duplicated so each gets the appropriate comment.


At this point, nailing down the copyright is about all that's left before I can merge it, I think.

@MadisonianX
Copy link
Author

I double-checked and the wind icon was from flaticons.com when I paid for a month a while back, so no attribution is required from their perspective. If it's still required on this end, what you added above looks fine minus the astronomical scanner.

If I can't get to finishing a new Astronomical Scanner image soon, I assume we can just merge it using the tactical scanner as a placeholder like all the other scanners added in Delta. But I do want to create something new and original for it so I can move on to my next PRs.

@Zitchas
Copy link
Member

Zitchas commented Jan 21, 2025

OK, thanks for the update! Yes, we could use the tactical scanner as a placeholder.

With the icon coming from flaticons.com via purchase, then that's good for me. We should still document it just so that it is clear where it came from, but if your subscription/purchase gives you the right to claim it as yours, then that's all that is required. (technically speaking, if something isn't claimed, then it gets claimed by MZ by default. Even things that are completely free and open for unrestricted use need to be documented)

Just taking a look at the Flaticon.com site in their licensing section, they state that, as you said, no crediting of the author is required. Which seems weird to me, but they get to set the terms for their product.

@Zitchas
Copy link
Member

Zitchas commented Jan 21, 2025

Just recording it here for archival / future reference:

https://support.flaticon.com/s/article/Can-I-keep-using-Flaticon-resources-once-my-subscription-has-expired?language=en_US

Flaticon No crediting required statement

Once you obtain any Flaticon subscription plan, you will have access to download a PDF license document every time you download an icon or pack/collection. These licenses will allow you to use Flaticon resources without crediting the author and will remain active even when your premium membership expires.

copyright Outdated Show resolved Hide resolved
@Zitchas Zitchas dismissed their stale review January 21, 2025 21:51

Because my review is outdated.

@Zitchas
Copy link
Member

Zitchas commented Jan 21, 2025

Alright, I updated the copyright to reflect the wind icon.

As for the other, I leave it up to you. If you can get it finished and included, great! If you decide you just want to use a placeholder for now and PR the image separately later, then just remove the image and update the outfit & copyright accordingly. I will be happy to merge it in either case.

Just ignore the check error. It is a spelling error in areas unrelated to this.

@Zitchas
Copy link
Member

Zitchas commented Jan 22, 2025

And that should get rid of the spelling mistakes...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Things that focus on the mechanics and internals of how they have engine works. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants